C - Conditional jump or move depends on uninitialised value(s)

Question

I'm getting this error from Valgrind when I try to run my program :

==23152== Conditional jump or move depends on uninitialised value(s)
==23152==    at 0x4C2D8D0: strcmp (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==23152==    by 0x40096C: str_lower_cmp (functions.c:41)
==23152==    by 0x400BB8: list_sort (list_sort.c:34)
==23152==    by 0x400CC7: get_wdir_content (working_dir.c:27)
==23152==    by 0x400C27: main (main.c:18)
==23152==  Uninitialised value was created by a heap allocation
==23152==    at 0x4C2C27B: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==23152==    by 0x400D4C: xmalloc (xfunctions.c:35)
==23152==    by 0x400886: lower_string (functions.c:20)
==23152==    by 0x400945: str_lower_cmp (functions.c:39)
==23152==    by 0x400BB8: list_sort (list_sort.c:34)
==23152==    by 0x400CC7: get_wdir_content (working_dir.c:27)
==23152==    by 0x400C27: main (main.c:18)
==23152== 
==23152== Conditional jump or move depends on uninitialised value(s)
==23152==    at 0x400BBB: list_sort (list_sort.c:34)
==23152==    by 0x400CC7: get_wdir_content (working_dir.c:27)
==23152==    by 0x400C27: main (main.c:18)
==23152==  Uninitialised value was created by a heap allocation
==23152==    at 0x4C2C27B: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==23152==    by 0x400D4C: xmalloc (xfunctions.c:35)
==23152==    by 0x400886: lower_string (functions.c:20)
==23152==    by 0x400945: str_lower_cmp (functions.c:39)
==23152==    by 0x400BB8: list_sort (list_sort.c:34)
==23152==    by 0x400CC7: get_wdir_content (working_dir.c:27)
==23152==    by 0x400C27: main (main.c:18)
==23152==

Though I can't say what is wrong, I'm guessing it comes from list_sort.c :

t_llist  *list_sort(t_llist *list)
{
    struct s_node *tmp;

    tmp = list->head;
    while (tmp != NULL)
    {
        if (tmp->next != NULL)
        {
            if (!tmp->name || !tmp->next->name)
                printf("Reached.\n");
            if (str_lower_cmp(tmp->name, tmp->next->name) > 0)
            {
                data_swap(tmp, tmp->next);
                tmp = list->head;
            }
            else
                tmp = tmp->next;
        }
        else
            return (list);
    }
    return (list);
}

Does that mean that at some point, the tmp->name or tmp->next->name value is uninitialised ?

EDIT (the functions.c code)

    char            *lower_string(char *s)
{
  char          *res;
  int           i;

  i = 0;
  res = xmalloc(sizeof(*res) * strlen(s) + 1);
  while (s[i])
    {
      if (s[i] >= 'A' && s[i] <= 'Z')
        res[i] = s[i] + 32;
      else
        res[i] = s[i];
      i++;
    }
  s[i] = '\0';
  return (res);
}

int             str_lower_cmp(char *s1, char *s2)
{
  char          *tmp1;
  char          *tmp2;
  int           res;

  tmp1 = lower_string(s1);
  tmp2 = lower_string(s2);
  res = strcmp(tmp1, tmp2);
  free(tmp1);
  free(tmp2);
  return (res);
}

Hmmm, what happens if you check the list pointer before using it? — LostBoy, Nov 20 '13 at 14:29
Since the report is in `strcmp` you can deduce you're allocating memory for your nodes, which is pattern-flling with the valgrind debug-allocator, then said-pointer-values are checked for that pattern by their debug strcmp. In other words, *yes*, you're likely sending an indeterminate `name` parameter either by value or by never properly filling and terminating the actual data it points to. Without knowing how you actually construct your list, its impossible to say how you're making that mistake. — WhozCraig, Nov 20 '13 at 14:35
Yes, it even tells you where you malloc'ed the string but did not fill it in: lower_string functions.c:20 — Charlie Burns, Nov 20 '13 at 14:37
I edited my post with a simple 'if' to test wether or not name and next->name are empty, and this code is never reached, so I suppose that I'm sending correct informations. — Kernael, Nov 20 '13 at 14:41
@Kernael That won't do it. The error is in code we can't see. post `lower_string` — WhozCraig, Nov 20 '13 at 14:43
Isn't it better to do the `(x)malloc` in `str_lower_cmp` instead of `lower_string`, this puts `str_lower_cmp` in charge of the `malloc` instead of some 'other' code. — Daan Timmer, Nov 20 '13 at 14:59
@DaanTimmer honestly it would better to not do it *at all*, considering it isn't even needed. — WhozCraig, Nov 20 '13 at 15:14

jcm · Answer 1 · 2013-11-20T14:50:30.800

3

Initially valgrind is telling you that you're running strcmp with a memory address allocated with malloc, coming from function lower_string, but that has no initial value assigned.

This would mean an undefined behavior, meaning that, depending on your code, can be very dangerous because can lead to unexpected results.

I would suggest using calloc in lower_string.

Edit: your're setting s[i] to 0 instead of res[i] (the pointer you've allocated and returning). On the other hand, I would suggest using calloc and checking res!=NULL

edited Nov 20 '13 at 14:50

answered Nov 20 '13 at 14:37

jcm

2,568
14
18

@WhozCraig I agree. Updated response ;). But, just to clarify, I meant that, with `strcmp`, most probably, it would return immediately and you would enter the `else` block (I assume there's an `if/else` checking `strcmp` response) – jcm Nov 20 '13 at 14:46
I'm calling malloc inside my xmalloc function, where I check its return. – Kernael Nov 20 '13 at 14:52
@Kernael Then, just call `calloc` instead of `malloc` and you will get all allocated memory initialized to 0 – jcm Nov 20 '13 at 14:53
@Kernael Was this useful to fix your warning message? – jcm Nov 25 '13 at 10:26

score 1 · Answer 2 · answered Nov 20 '13 at 14:46

Your error is here in lower_string You're not terminating the string you're allocating:

char *lower_string(char *s)
{
    char *res;
    int i;

    i = 0;
    res = xmalloc(sizeof(*res) * strlen(s) + 1);
    while (s[i])
    {
        if (s[i] >= 'A' && s[i] <= 'Z')
            res[i] = s[i] + 32;
        else
            res[i] = s[i];
        i++;
    }
    s[i] = '\0'; // THIS IS WRONG
    return (res);
}

The marked line should be this:

    res[i] = '\0'; // THIS IS RIGHT

And note, this would be caught if you properly passed the input string as a const parameter:

char *lower_string(const char *s) // MAKE PARAM CONST

Doing so would have failed to compile because your s[i] = '\0' assignment would have violated the const-condition. General rule, unless you need to modifying something passed as a a by-address parameter, make it const

Thank you, it was such a dumb mistake at the end. Thank you for the tip too, I'll use it. — Kernael, Nov 20 '13 at 14:48
@Kernael if you think that is "dumb" you will be most-displeased to know you can implement `str_lower_cmp` with no dynamic allocation at all, thereby making the problem a non-issue to begin with. Consider that. — WhozCraig, Nov 20 '13 at 14:51

score 0 · Answer 3 · answered Nov 20 '13 at 14:51

0

when the "char *s" passed to lower_string is an empty string you'd have a crashing program too. Calling calloc as jcm said would help fix that problem

answered Nov 20 '13 at 14:51

Serve Laurijssen

9,266
5
45
98

C - Conditional jump or move depends on uninitialised value(s)

3 Answers3