I have two files with some C functions called users.h and users.c. I have a test file called testUsers.c. I am using valgrind to find memory leaks and errors. I am getting a lot of "Conditional jump or move depends on unitialised value(s)" errors. I don't see the uninitialized values.
For example:
users.h:
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#define MAX_USERS 10
#define MAX_USER_NAME_SIZE 20
#define PERMISSION_LAUNCH 4
#define PERMISSION_EDIT_USER 2
#define PERMISSION_EDIT_LAUNCHER 1
struct User {
char user_name[20];
char password[20];
int permissions;
int index;
};
typedef struct {
int code;
char *message;
char* payload;
} Error;
extern char* user_names[MAX_USERS][MAX_USER_NAME_SIZE];
Error create_error(int code, char *message);
Error createUser(char* user_name, char* password, int permissions);
struct User* findUser(char * user_name);
void printUsers();
Error initDB();
users.c:
#include <stdio.h>
#include <stdbool.h>
#include <stdint.h>
#include <string.h>
#include <stdlib.h>
#include "users.h"
/* An array of pointers to the User structs created */
static struct User* users_storage[MAX_USERS];
/* The number of users in the users_storage array */
int user_counter = 0;
/*
A struct to return error messages to the UI as needed for errors that come up.
There are no different error codes, just 0 for success and -1 for an error. The UI
Would check the error code and then display the error message.
*/
Error create_error(int code, char *message) {
Error err;
err.code = code;
err.message = message;
err.payload = "Nothing to see here.";
return err;
}
/*
Null out the users_storage and add the "admin" user to the users_storage
*/
Error initDB() {
Error result = create_error(0, "Success");
for (int i = 0; i < MAX_USERS; i++) {
if (users_storage[i] != NULL) {
free(users_storage[i]);
}
users_storage[i] = NULL;
}
result = createUser("admin", "password", PERMISSION_LAUNCH + PERMISSION_EDIT_USER + PERMISSION_EDIT_LAUNCHER);
return result;
}
/*
Create a new User.
- Checks for invalid permissions; it must be between 1 and 7
- Checks for duplicate users using the user name
- Creates a new user and adds it to the users_storage
- Increments the user_counter
- If the users_storage is full (ie there are MAX_USERS in the users_storage), then an error is returned
with a message to the UI that a user must be deleted before a new one can be created.
*/
Error createUser(char* user_name, char* password, int permissions) {
int user_permissions = 0;
Error result = create_error(0, "Success");
if (permissions < 0 || permissions > 7) {
char error_message[100] = {0};
sprintf(error_message, "Invalid permissions: %i. Permissions must be between 0 and 7. Permissions set to 1.", permissions);
result.code = -1;
result.message = error_message;
user_permissions = 1;
}
else {
user_permissions = permissions;
}
struct User* duplicate = findUser(user_name);
if (duplicate != NULL) {
result.message = "Duplicate user";
result.code = -1;
return result;
}
struct User* user = findUser(NULL);
if (user != NULL) {
strcpy(user->user_name, user_name);
strcpy(user->password, password);
user->permissions = user_permissions;
user_counter++;
return result;
}
else {
result.message = "Out of user memory. Delete a user before creating another one.";
result.code = -1;
return result;
}
}
/*
Finds a user by user name.
- Returns NULL if the user does not exist
- Returns the User struct if the user is found
- Note: if a user has been previously deleted, then there is a "hole" in the users_storage.
findUser will find that "hole" before the end of the users_storage, so holes
left by deleted users are filled with new users.
*/
struct User* findUser(char* user_name) {
for (int i = 0; i < MAX_USERS; i++) {
struct User * user = users_storage[i];
if (user == NULL && user_name == NULL) {
// found and empty user slot
struct User * user = malloc(sizeof(struct User));
user->index = i;
users_storage[i] = user;
return user;
}
if (user != NULL && user_name != NULL) {
if (strcmp(user->user_name, user_name) == 0) {
return user;
}
}
}
return NULL;
}
/*
Convenience function to print out the contents of the users_storage in a human
readable format for debugging.
*/
void printUsers() {
for (int i = 0; i < MAX_USERS; i++) {
struct User* user = users_storage[i];
if (user != NULL) {
printf("%i, %s, %s, %i, %i\n", i, user->user_name, user->password, user->permissions, user->index);
}
else {
printf("%i, NULL\n", i);
}
}
}
The testUser.c code:
#include <stdlib.h>
#include "users.h"
int main() {
printf("InitDB: ");
Error result = initDB();
printf("%i, %s\n",result.code, result.message);
printUsers();
//create a user with bad permissions
printf("\ncreate a user ron with bad permissions (9): ");
result = createUser("ron", "password", 9);
printf("%i, %s\n",result.code, result.message);
printUsers();
// create a user
printf("\ncreate john: ");
result = createUser("john", "pw0", 4);
printf("%i, %s\n",result.code, result.message);
printUsers();
return(0);
}
Makefile:
usermake: users.c testUsers.c
gcc -o -g -O0 -o testUsers users.c testUsers.c -I .
output from valgrind -s --track-origins=yes --leak-check=full --show-leak-kinds=all ./testUsers
InitDB: 0, Success
0, admin, password, 7, 0
1, NULL
2, NULL
3, NULL
4, NULL
5, NULL
6, NULL
7, NULL
8, NULL
9, NULL
==624208== Conditional jump or move depends on uninitialised value(s)
==624208== at 0x484ED19: strlen (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==624208== by 0x48EBD30: __vfprintf_internal (vfprintf-internal.c:1517)
==624208== by 0x48D579E: printf (printf.c:33)
==624208== by 0x10AE29: main (in /home/mark/ESP32-Projects/esp32-launcher/sandbox/users/testUsers)
==624208== Uninitialised value was created by a stack allocation
==624208== at 0x109160: ??? (in /home/mark/ESP32-Projects/esp32-launcher/sandbox/users/testUsers)
==624208==
==624208== Conditional jump or move depends on uninitialised value(s)
==624208== at 0x484ED28: strlen (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==624208== by 0x48EBD30: __vfprintf_internal (vfprintf-internal.c:1517)
==624208== by 0x48D579E: printf (printf.c:33)
==624208== by 0x10AE29: main (in /home/mark/ESP32-Projects/esp32-launcher/sandbox/users/testUsers)
==624208== Uninitialised value was created by a stack allocation
==624208== at 0x109160: ??? (in /home/mark/ESP32-Projects/esp32-launcher/sandbox/users/testUsers)
==624208==
==624208== Conditional jump or move depends on uninitialised value(s)
==624208== at 0x4900737: _IO_new_file_xsputn (fileops.c:1218)
==624208== by 0x4900737: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1196)
==624208== by 0x48EC00B: outstring_func (vfprintf-internal.c:239)
==624208== by 0x48EC00B: __vfprintf_internal (vfprintf-internal.c:1517)
==624208== by 0x48D579E: printf (printf.c:33)
==624208== by 0x10AE29: main (in /home/mark/ESP32-Projects/esp32-launcher/sandbox/users/testUsers)
==624208== Uninitialised value was created by a stack allocation
==624208== at 0x109160: ??? (in /home/mark/ESP32-Projects/esp32-launcher/sandbox/users/testUsers)
==624208==
==624208== Syscall param write(buf) points to uninitialised byte(s)
==624208== at 0x4989887: write (write.c:26)
==624208== by 0x48FFEEC: _IO_file_write@@GLIBC_2.2.5 (fileops.c:1180)
==624208== by 0x49019E0: new_do_write (fileops.c:448)
==624208== by 0x49019E0: _IO_new_do_write (fileops.c:425)
==624208== by 0x49019E0: _IO_do_write@@GLIBC_2.2.5 (fileops.c:422)
==624208== by 0x49006D4: _IO_new_file_xsputn (fileops.c:1243)
==624208== by 0x49006D4: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1196)
==624208== by 0x48EAFC9: outstring_func (vfprintf-internal.c:239)
==624208== by 0x48EAFC9: __vfprintf_internal (vfprintf-internal.c:1593)
==624208== by 0x48D579E: printf (printf.c:33)
==624208== by 0x10AE29: main (in /home/mark/ESP32-Projects/esp32-launcher/sandbox/users/testUsers)
==624208== Address 0x4aa1070 is 48 bytes inside a block of size 1,024 alloc'd
==624208== at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==624208== by 0x48F3BA3: _IO_file_doallocate (filedoalloc.c:101)
==624208== by 0x4902CDF: _IO_doallocbuf (genops.c:347)
==624208== by 0x4901F5F: _IO_file_overflow@@GLIBC_2.2.5 (fileops.c:744)
==624208== by 0x49006D4: _IO_new_file_xsputn (fileops.c:1243)
==624208== by 0x49006D4: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1196)
==624208== by 0x48EA14C: outstring_func (vfprintf-internal.c:239)
==624208== by 0x48EA14C: __vfprintf_internal (vfprintf-internal.c:1263)
==624208== by 0x48D579E: printf (printf.c:33)
==624208== by 0x10AD89: main (in /home/mark/ESP32-Projects/esp32-launcher/sandbox/users/testUsers)
==624208== Uninitialised value was created by a stack allocation
==624208== at 0x109160: ??? (in /home/mark/ESP32-Projects/esp32-launcher/sandbox/users/testUsers)
==624208==
create a user ron with bad permissions (9): -1, Invalid permissions: 9. Permissions must be between 0 and 7. Permissions set to 1.
0, admin, password, 7, 0
1, ron, password, 1, 1
2, NULL
3, NULL
4, NULL
5, NULL
6, NULL
7, NULL
8, NULL
9, NULL
create john: 0, Success
0, admin, password, 7, 0
1, ron, password, 1, 1
2, john, pw0, 4, 2
3, NULL
4, NULL
5, NULL
6, NULL
7, NULL
8, NULL
9, NULL
What I don't understand. If I remove the section in testUsers.c that starts with the comment //create a user with bad permissions and go right to the section that starts with
// create a user, valgrind does not complain. The error only occurs if I leave in the section that starts with //create a user with bad permissions.
The "additonal code" that runs in Error createUser(char* user_name, char* password, int permissions) when trying to create a user with invalid permissions starts with the if clause below:
Error result = create_error(0, "Success");
int user_permissions = 0;
if (permissions < 0 || permissions > 7) {
char error_message[100] = {0};
sprintf(error_message, "Invalid permissions: %i. Permissions must be between 0 and 7. Permissions set to 1.", permissions);
result.code = -1;
result.message = error_message;
user_permissions = 1;
}
else {
user_permissions = permissions;
}
I don't see any un-initialized variables in that section of code. What am I missing? I am also confused by valgrind producing its error message in between two function calls in testUsers.c.
In createUser you have:
if (permissions < 0 || permissions > 7) {
char error_message[100] = {0};
sprintf(error_message, "Invalid permissions: %i. Permissions must be between 0 and 7. Permissions set to 1.", permissions);
result.code = -1;
result.message = error_message;
user_permissions = 1;
}
The lifetime of the array error_message is only the block in which it is defined. You set the pointer result.message to point to this array, but that means that after exiting this block, you cannot use that pointer to access the array anymore.
If you want the array to survive outside the block, you need to allocate it a different way, such as via malloc:
char *error_message = malloc(100);
sprintf(error_message, "Invalid permissions: %i. Permissions must be between 0 and 7. Permissions set to 1.", permissions);
result.message = error_message;
Now result.message is usable until you free() it (which you must make sure to do when you no longer need it).
(If your system provides the non-standard asprintf function, you might consider using it, as it will make sure the buffer is of the correct size for the data being formatted into it.)
You wrote in a comment
I put the error_message in the Error struct
You put a pointer to error_message in the Error struct. That doesn't make error_message live any longer than it otherwise would.
and testUser.c prints out the error message from the Error struct, so I thought it was working and correct C code.
As a new C programmer, you'll need to learn that you cannot reason like "it works in my test, therefore it's correct". Many types of erroneous C code cause undefined behavior; although they are wrong, the compiler is not required to ensure that they fail in all cases. So it often happens that such code will appear to work correctly in simple tests, but it is entirely possible that it will fail unexpectedly in later tests, even if nothing else has changed.
This is why tools like valgrind are so valuable; they perform more stringent checking, so that they can find such errors even when they don't appear in other tests. So you've already learned the other important lesson, which is to make use of such tools as much as possible. Its error message here is a little bit misleading, but it did correctly identify that the string does not (necessarily) contain what you thought it did.