Conditional jump or move depends on uninitialised value(s) at strcpy

Question

Valgrind detects a problem with strcpy in the following code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main () {

    char **array;
    int mallocedLen = 15;
    int arrLen = 10;
    char tempArr[20][20] = {"abc", "123", "def", "456", "ghi", "789"};
    int ii;
    
    array = (char**)malloc(sizeof(char*) * arrLen);
    for(ii = 0; ii < arrLen; ii++){
        array[ii] = (char*)malloc(sizeof(char) * (strlen(tempArr[ii]) + 1));
        strcpy(tempArr[ii], array[ii]);
        array[ii][strlen(tempArr[ii])] = '\0';
        mallocedLen++;
    }
    
    return 0;
}

==4360== Conditional jump or move depends on uninitialised value(s)

==4360== at 0x483F0A2: strcpy (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)

The code seems to compile fine so I don't think it's a problem with mallocing.

`strcpy`'s params are in the order `dest, src`. Did you mean to write `strcpy(array[ii], tempArr[ii]);` instead of `strcpy(tempArr[ii], array[ii]);`. — mediocrevegetable1, Jun 11 '21 at 15:26
Style point: What is "temporary" about `tempArr`. All the other variables declared in that scope have identical lifetime. The implication in a name that a variable is temporary should be avoided. Often it is used to "justify" reuse of one variable for several unrelated purposes - that is a bad idea in itself, but here it is not even used in that manner. — Clifford, Jun 11 '21 at 15:55
_"The code seems to compile fine so I don't think it's a problem with mallocing."_ Dynamic memory allocation occurs at runtime, so that statement is non-sequitur. Valgrind's primary purpose is to detect _"problems with mallocing"_, if the compiler could detect that, Valgrind would not exist. The compiler reports syntactic errors and can in some cases _warn_ of semantic errors detectable through static or idiomatic analysis - potential runtime errors however are less easy to detect statically. — Clifford, Jun 11 '21 at 16:12

Clifford · Accepted Answer · 2021-06-11T15:56:51.843

Here:

 array[ii] = (char*)malloc(sizeof(char) * (strlen(tempArr[ii]) + 1));
 strcpy(tempArr[ii], array[ii]);

You allocate space to array[ii], then immediately pass it as a source string to strcpy() when it does not yet contain a valid string.

It seems likely that you intended tempArr[ii] as the source:

strcpy( array[ii], tempArr[ii] ) ;

And the nul termination is unnecessary - strcpy() does that already.

Conditional jump or move depends on uninitialised value(s)

Means what it says. For example strcpy() might have:

while( *source != 0 )
{
    ...
}

But *source has not been initialised, so the loop may or may not iterate.

Note also that had you declared tempArr as:

const char* tempArr[] = {"abc", "123", "def", "456", "ghi", "789"};

the compiler would have caught your error. I assume tempArr is immutable? In which case declare it as such. It is always better to trap semantic errors at build time rather then rely on runtime detection tools. It is also more memory efficient in this case.

Another issue is :

for(ii = 0; ii < arrLen; ii++){

where arrlen is 10 but tempArr has only 6 initialisers. Better to use a sentinal value such as NULL or an empty string. Or if you use the const char* tempArr[] declaration above, then:

const char* tempArr[] = {"abc", "123", "def", "456", "ghi", "789"}; 
const int arrLen = sizeof(tempArr) / sizeof(*tempArr) ;

Thank you for your ans! I realize I should have specified that this code was taken out of a larger source code which is why the initialisations don't make sense. I was also for some reason led to think that the problem was with mallocing. — V. Loh, Jun 12 '21 at 02:47
@V.Loh It is in the sense that the allocated space was used uninitialised. This code cannot have worked in any case, so regular testing and debugging should have found the error. Valgrind is great at finding subtle memory bugs, but this should have been obvious. When presenting minimised code, truly minimise it, removing everything not necessary to the problem, otherwise the flaws are a distraction. — Clifford, Jun 12 '21 at 06:25

score 0 · Answer 2 · answered Jun 11 '21 at 15:29

0

It's strcpy(destination, source). Also strcpy() 0-terminates the copy.

So replace

  strcpy(tempArr[ii], array[ii]);
  array[ii][strlen(tempArr[ii])] = '\0';

by

  strcpy(array[ii], tempArr[ii]);

answered Jun 11 '21 at 15:29

alk

69,737
10
105
255

Any information on why that particular error message was chosen? – Tim Randall Jun 11 '21 at 15:29
@TimRandall Using the uninitialised memory as source, causes undefined behaviour the moment it is read for example to run tests against it content. – alk Jun 11 '21 at 15:33
1

@Tim the error message refers to an "conditional jump" occurring _inside_ `strcpy()` - see my answer. – Clifford Jun 11 '21 at 15:39

Conditional jump or move depends on uninitialised value(s) at strcpy

2 Answers2