Uninitialised value was created by a heap allocation

Question

I have been chasing this bug around, and I just don't get it. Have I forgotten some basic C or something?

==28357== Conditional jump or move depends on uninitialised value(s)
==28357==    at 0x4C261E8: strlen (mc_replace_strmem.c:275)
==28357==    by 0x4E9280A: puts (ioputs.c:36)
==28357==    by 0x400C21: handlePath (myshell.c:105)
==28357==    by 0x400B17: handleInput (myshell.c:69)
==28357==    by 0x400AAD: acceptInput (myshell.c:60)
==28357==    by 0x4009CF: main (myshell.c:33)
==28357==  Uninitialised value was created by a heap allocation
==28357==    at 0x4C25153: malloc (vg_replace_malloc.c:195)
==28357==    by 0x400BDE: handlePath (myshell.c:99)
==28357==    by 0x400B17: handleInput (myshell.c:69)
==28357==    by 0x400AAD: acceptInput (myshell.c:60)
==28357==    by 0x4009CF: main (myshell.c:33)
==28357==
(095) void handlePath(char *input) {
(096)     if(DEBUG_ON) { printf("%s%s\n", "DEBUG_HANDLEPATH: ", input); }
(097)
(098)     char *inputCopy = NULL;
(099)     inputCopy = (char *)malloc((strlen(input)+1)*sizeof(char));
(100)
(101)     if(inputCopy==NULL) {
(102)         die("malloc() failed in handlePath()");
(103)     }
(104)     strncpy(inputCopy, input, strlen(input)*sizeof(char));
(105)     printf("%s\n", inputCopy);
(106)     free(inputCopy);
(107)     return;
(108) }

Line 96 prints the parameter "char *input" just fine (DEBUG_ON==1), but line 105 spits out valgrind errors (it does print just fine in the console). "char *input" originates from a getline() grabbing a line of input, and in the case of this function will be something like "path /test/path" without quotes. I can print and manipulate it just fine in preceding functions. What's uninitialized about "char *inputCopy"? Any ideas? Thanks in advance!

Solution 1

You have two mistakes on line 104,

strncpy(inputCopy, input, strlen(input)*sizeof(char));

You need to give strncpy room for the terminating null, so it should be strlen(input)+1 strncpy isn't guranteed to leave the output buffer null terminated, which seems like a bug in strncpy but it isn't. It was designed to work that way. What strncpy was designed to do was copy a string into an output buffer and then fill the rest of the buffer with zeros., It's not really designed as a 'safe strcpy'

Your other bug is that strncpy takes a character count not a byte count, so it's incorrect to multiply by sizeof(char).. Since sizeof(char) == 1, this isn't actually causing problems, but its still the wrong intent.

You were correct to multiply by sizeof(char) in the malloc on line 99 since malloc needs a byte count.

Solution 2

strncpy will not put a terminating 0 character as it copies at most N characters (where N is the 3 parameter). Since you specified the length and did not include the +1 for the terminating 0, it was not added.

So assuming your have a buffer of N bytes, the proper use of strncpy is this:

strncpy(dest, src, N - 1);
dest[N - 1] = '\0';

strncpy is a strange function. Besides not promising to write a terminating 0, it will always write exactly N characters to the destination buffer. If src is smaller then N, strncpy will actually take the time to fill the entire rest of the buffer with 0's.

Solution 3

I believe your strncpy isn't putting a terminating null character at the end of the string, so printf is running off the end of the allocated memory.

Comments

[yavoh]

Silly me! A serious case of RTFM on my end. Thanks! That solved it nicely.

[ephemient]

sizeof(char)==1 by definition, but there's no harm in being explicit.

[John Knoeller]

@ephemient: Agreed, although I would have written sizeof(inputCopy[0]) rather than sizeof(char).

[GManNickG]

@ephemient: Expect that's really the same as putting 1 * in front of variables: useless and confusing. Better to keep clutter out, I think.

[Paul R]

Good catch. Additionally the strncpy should probably just be strcpy anyway.

[Secure]

p = malloc(Size*sizeof(*p)); -- In C there is no need for the cast. Now the statement works for any type of p without change.

[caf]

Since you know the exact number of bytes you want to copy, you can simply use memcpy.

[bk1e]

The sizeof(char) becomes a problem if you later add wide string support by replacing strncpy(...*sizeof(char)) with wcsncpy(...*sizeof(wchar_t)).