Valgrind Uninitialized Values (making a linked list data struct)

Question

I've created a linked list class, but this function is producing valgrind errors based saying that there is a conditional jump based upon an uninitialized value in this function. I'm not exactly sure what I need to do to fix it.

Essentially there is a node class for the linked list and this is iterating over all the nodes checking if the key parameter matches a pre-existing node and if it does it returns the value.

const char *dictionary_get(dictionary_t *d, const char *key)
{

node* current;
current = d->head;
if(strcmp(current->key,key)==0)
        return current->value;
while(current->next != NULL){
        current = current->next;
        if(current!=NULL && strcmp(current->key,key)==0)
                return current->value;
}

        return NULL;
}    

Any ideas?

---

I've reexamined with valgrind tracking origins and this is the output:

==25042== Conditional jump or move depends on uninitialised value(s)
==25042==    at 0x4A06E6A: strcmp (mc_replace_strmem.c:412)
==25042==    by 0x400DD6: dictionary_get (libdictionary.c:143)
==25042==    by 0x400826: main (part2.c:84)
==25042==  Uninitialised value was created by a stack allocation
==25042==    at 0x400AE3: dictionary_parse (libdictionary.c:69)
==25042== 
==25042== Conditional jump or move depends on uninitialised value(s)
==25042==    at 0x4A06E8A: strcmp (mc_replace_strmem.c:412)
==25042==    by 0x400DD6: dictionary_get (libdictionary.c:143)
==25042==    by 0x400826: main (part2.c:84)
==25042==  Uninitialised value was created by a stack allocation
==25042==    at 0x400AE3: dictionary_parse (libdictionary.c:69)
==25042== 
==25042== Conditional jump or move depends on uninitialised value(s)
==25042==    at 0x400DD9: dictionary_get (libdictionary.c:143)
==25042==    by 0x400826: main (part2.c:84)
==25042==  Uninitialised value was created by a stack allocation
==25042==    at 0x400AE3: dictionary_parse (libdictionary.c:69)

It looks like this might be coming from dictionary_parse so I'll post that function too.

int dictionary_parse(dictionary_t *d, char *key_value)
    {
char* colon;
char* space;
colon = key_value;
space = key_value;

space++;

int key_length = -1; //Default key length to check for failure

int i=0;
int j=0;   // Loop variables
int k=0;

int length = strlen(key_value);

for(i=0;i<length-2;i++){
        if(*colon == ':' && *space == ' '){
                key_length = i;
                break;
        }
        colon++;
        space++;
}

if(key_length == -1 || key_length == 0)
        return -1;

int value_length = length-2-key_length;

colon = key_value;


char key_word[key_length];
key_word[0] = '\0';
char value_word[value_length];
value_word[0] = '\0';

for(j=0;j<key_length;j++){
key_word[j] = *colon;
colon++;
}

space++;    

for(k=0; k<value_length;k++){
value_word[k] = *space;
space++;
    }
char* finalkey[key_length];
strcpy((char*)finalkey,key_word);
char* finalvalue[value_length];
strcpy((char*)finalvalue,value_word);

dictionary_add(d,(char*)finalkey,(char*)finalvalue);

return 0;
}

Answer 1

If your program works correctly, don't bother about those warnings. I've seen the conditional jump warning on programs that otherwise work perfectly. It probably has to do with the assembly code generated by the compiler and not directly related to your code.

Answer 2

Lines like

char key_word[key_length];

look highly suspicious.

I don't know what you do with these further along, but creating a temporary variable length array for things that should be persistent for longer than the function call seems very strange.

Also, the variable length array does not include the terminating '\0'.

Answer 3

You are not properly null-terminating the strings in key_word and value_word, and this error is apparently propagating through. This loop is the problem:

for(j=0;j<key_length;j++){
  key_word[j] = *colon;
  colon++;
}

It copies key_length characters into key_word, but none of those copied characters is a null-terminator. You can fix the problem by adding one extra byte to key_word:

char key_word[key_length + 1];

Then adding this after the for() loop:

key_word[key_length] = '\0';

There is also no need to create the copies in finalkey and finalvalue (which have the wrong type, anyway - which is why you end up needing all those ugly casts). So overall it would look like this:

char key_word[key_length + 1];
char value_word[value_length + 1];

for (j = 0; j < key_length; j++) {
  key_word[j] = *colon;
  colon++;
}
key_word[key_length] = '\0';

space++;    

for(k = 0; k < value_length; k++) {
  value_word[k] = *space;
  space++;
}
value_word[value_length] = '\0';

dictionary_add(d, key_word, value_word);

Really though, you should simplify this function using the facilities from string.h. For example, strstr() will let you search for the ": " string that separates your key and value, and memcpy() does the equivalent of those for() loops:

int dictionary_parse(dictionary_t *d, char *key_value)
{
    char *colon;
    char *value;
    int key_length = -1; //Default key length to check for failure

    colon = strstr(key_value, ": ");

    if (colon != NULL) {
        key_length = colon - key_value;  // Number of characters before the colon
        value = colon + 2;  // Value is portion of the string after ": "
    }

    if (key_length < 1) {
        return -1;
    }

    char key_word[key_length + 1];
    memcpy(key_word, key_value, key_length);
    key_word[key_length] = '\0';

    dictionary_add(d, key_word, value);

    return 0;
}