uninitialized value was created by a heap allocation. why?

Question

After running Valgrind on my code I got an error:

uninitialized value was created by a heap allocation.

My code:

void adicionaHashtag(char* x){
    char*y=malloc(sizeof(x));/***ERROR IS HERE***/
    int i;
    for(i=0; i<strlen(x); i++){
        y[i]=tolower(x[i]);
    }

    Lista_Hashtags*aux = (Lista_Hashtags*)malloc(sizeof(Lista_Hashtags));
    strcpy(aux->nome, y);
    aux->contador=1;
    if(contador_unitario == 0){
        ultimo = aux;
    }
    contador_unitario++;
    aux->proximo = primeiro;
    primeiro = aux;
}

any tips?

also, what does "Conditional jump or move depends on uninitialised value(s)" mean? ~

Use of `sizeof(x)` is incorrect there . Allocate memory equal to `strlen(x)+1` to `y` . — ameyCU, May 11 '16 at 17:29
Also, don't forget to add `\0` to `y` or `strcpy` will have problems. — 001, May 11 '16 at 17:33
sorry in advance for the "noob" question. How exacly should I add \0 to y? strcpy is indeed giving trouble. @Johny Mopp — Vasco Morganho, May 11 '16 at 17:39
Ok! I changed it, but the error doesn't seem to be resolved.. thanks for the tip though. @ameyCU — Vasco Morganho, May 11 '16 at 17:40
`char* y = strdup(x);` would have prevented both of these problems. Some people might argue that it would be less efficient (going over the string twice - once to copy `x`, then again to convert to lowercase), but unless we're talking about huge strings being processed many times, the overhead is probably worthwhile: making the code simpler and more obviously correct for what's probably minuscule runtime cost.. — Michael Burr, May 11 '16 at 17:42
Another possibility for some of the valgrind messages might be that the argument `x` isn't being passed a valid string. If the suggestions here don't solve your problem there might also be a problem at the location(s) where `adicionaHashtag()` is called. — Michael Burr, May 11 '16 at 17:44
@MichaelBurr Thanks a lot Michael! "strdup" did indeed help!! Thanks again my friend, a good one! — Vasco Morganho, May 11 '16 at 17:47
Also, `strcpy(aux->nome, y);` looks like a potential problem. I wouldn't be surprised if it should be `aux->nome = y;`, but that really depends on what type `Lista_Hashtags.nome` is. If it's a `char*` then you want `aux->nome = y`, if it's a `char[]` then you want something like `strcpy()`, but you may need to protect against buffer overrun. — Michael Burr, May 11 '16 at 17:50

score 2 · Accepted Answer · answered May 12 '16 at 01:29

2

"uninitialized value ..." You may use char*y=calloc(strlen(x),0); instead of char*y=malloc(sizeof(x));.Initialize the memory with "0".

answered May 12 '16 at 01:29

051026luyaok

191
5

Or you can initialize the memory after allocating it. All-bits-zero may or may not be a valid initial value. For example, null pointers are usually represented as all-bits-zero, but that's not actually guaranteed. – Keith Thompson May 12 '16 at 01:36
2

size needs +1 for the null terminator – M.M May 12 '16 at 01:38
@KeithThompson all bits zero is valid for `char` as it is an integer type – M.M May 12 '16 at 01:39
@M.M: Yes, but we don't know what's in `Lista_Hashtags` -- though now that I look at it, it's the first `malloc` that's apparently the problem. On the other hand, if an allocated array of `char` is going to hold a string, there's no need to zero all of it. – Keith Thompson May 12 '16 at 01:42
Yet another example of an allocation for a string that is too small for the terminating null. Please - can people just start using `strdup()` so we can stop seeing this bug over and over? – Michael Burr May 12 '16 at 01:54

Keith Thompson · Answer 2 · 2016-05-12T02:23:49.373

void adicionaHashtag(char* x){
    char*y=malloc(sizeof(x));/***ERROR IS HERE***/
    int i;
    for(i=0; i<strlen(x); i++){
        y[i]=tolower(x[i]);
    }

You're trying to allocate and initialize a string with a copy of the string pointed to by x.

sizeof(x) is the size of a char* pointer. That's typically 4 or 8 bytes. You need to allocate enough space to hold the string itself.

char *y = malloc(strlen(x) + 1);

The + 1 is to allow for the terminating null character '\0'.

Calling strlen on each iteration of the loop is inefficient, but not incorrect; I'll leave that for now. There's a potential problem if any of the copied characters have negative values (an unfortunate characteristic of the tolower() function. The assignment should be:

y[i] = tolower((unsigned char)x[i]);

Finally, let's say x points to the string "hello". You're correctly copying 5 characters -- but not the final '\0'. One possible fix is to change the < in the loop condition to <=, which will copy characters 0 through 5 rather than 0 through 4.

But you can just use the existing strcpy function, which handles all this for you (and more efficiently):

char *y = malloc(strlen(x) + 1);
strcpy(y, x);

(You should also check the value returned by malloc, and treat it as an error if it's NULL.)

For that matter, you probably don't need y at all. x is already a pointer to a string. You copy it into the memory allocated for y, and then you copy that into aux->nome. Unless there's more code you haven't shown us that uses y, this is unnecessary (and a memory leak!). You can drop the declaration of y and the code that initializes it, and just copy from x directly:

Lista_Hashtags *aux = malloc(sizeof *aux);
strcpy(aux->nome, x);

(This assumes that aux->nome is an array, not a pointer, and that it's big enough to hold a copy of the string.)

(Note that I've changed your malloc call to a simpler and more robust form.)

@MichaelBurr: Note that `strdup` is non-standard. – Keith Thompson May 12 '16 at 02:23 — Keith Thompson, May 12 '16 at 02:23

uninitialized value was created by a heap allocation. why?

2 Answers2