Conditional jump or move error in valgrind

Question

I am trying to use valgrind to detect memory errors. This is part of my code-

else
{
137.    printf("HELlo\n");  
138.    char * lexeme1;char * lexeme2;
139.    lexeme1=substr1(bufferOld,beginPointer,sizeBuffer-1);
140.    lexeme2=substr1(buffer,0,indexStart-1);
141.    strcat(lexeme,lexeme1);
142.    strcat(lexeme,lexeme2);
}

Token  getNextToken( int fp1, FILE * fp)
{
  ...
  207. lexeme=(char *)malloc(sizeof(char) * 100);
  ...
}

Upon running valgrind it gives me the following error-

==9720== Conditional jump or move depends on uninitialised value(s)
==9720==    at 0x4C2DD9A: strcat (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9720==    by 0x401048: updateToken (lexer.c:141)
==9720==    by 0x402A92: getNextToken (lexer.c:498)
==9720==    by 0x400A17: main (driver.c:66)
==9720==  Uninitialised value was created by a heap allocation
==9720==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9720==    by 0x4012C6: getNextToken (lexer.c:207)
==9720==    by 0x400A17: main (driver.c:66)
==9720==

I am not sure why I am getting these kinds of error. Any help would be highly appreciated.

Update-

Here is my substr1 function-

char * substr1(char * source,int start,int end)
{

    char * dest=malloc((end-start+2)*sizeof(char));
    if(end==-1)
        return dest;
    int i,count=0;
    for(i=start;i<=end;i++)
        dest[count++]=source[i];
    dest[count]='\0';
    return dest;
}

Take a look at http://stackoverflow.com/questions/2612447/pinpointing-conditional-jump-or-move-depends-on-uninitialized-values-valgrin and see if that helps? — wilkesybear, Mar 22 '16 at 19:27
In the posted fragments, you have allocated memory for `lexeme` but not initialised it as a C string so that it can be the object of `strcat`. — Weather Vane, Mar 22 '16 at 19:40
Also `if (end==-1)` case probably wants to set `dest[0]='\0';` before returning. — Rup, Mar 22 '16 at 19:42
@ryyker I am not `free`ing `dest`. Should that be causing this error? — Noober, Mar 22 '16 at 19:42
@Noober NO, but that is a problem too. You are using `strcat(lexeme,lexeme1);` and `leseme` is not initialized — Michi, Mar 22 '16 at 19:43
Show us lines `66, 498` by the way The line `66` is inside driver.c =>> `driver.c:66` — Michi, Mar 22 '16 at 19:45
@Michi Using `strcpy(lexeme,lexem1)` for the first time fixed the error. Thanks a lot. It seems `lexeme` was not initialized and that was causing the error. — Noober, Mar 22 '16 at 19:47
@Noober `strcat` allows one memory block to be appended to another memory block. Both memory blocks are required to be null-terminated in your case wasn't so, hence the need of `strcpy`, Remember that. Any way I'm not saying that the two Answers are wrong, but the problem with `calloc` is, that ends with too many `ZERO's` — Michi, Mar 22 '16 at 19:59
in C, when calling `malloc()` 1) do not cast the returned value. The return type is `void*` so can be assigned to any pointer. Casting just clutters the code making it more difficult to understand, debug, maintain. 2) the expression: `sizeof( char)` is defined in the standard as 1. multiplying anything by 1 has no effect on the parameter passed to `malloc()` Strongly suggest removing that expression. 3) when calling `malloc()` always check (!=NULL) to assure the operation was successful — user3629249, Mar 23 '16 at 21:12
regarding this line: `char * dest=malloc((end-start+2)*sizeof(char));` with `end` having a possible value of -1 and start being at some offset into the char array, the value passed to `malloc()` will be negative, (but malloc() assumes a positive parameter) so a huge amount of memory will be allocated. probably not what you want. Suggest checking that `end` is greater than `start` before calling malloc()` — user3629249, Mar 23 '16 at 21:16
regarding this code block: `if(end==-1) return dest;`, this will return a pointer to uninitialized memory. Probably not what you want. If `malloc()` failed, then would be returning NULL, given the other code, probably not what you want. — user3629249, Mar 23 '16 at 21:19
for ease of readability and understanding by us humans: please follow the axiom: *only one statement per line and (at most) one variable declaration per statement.* — user3629249, Mar 23 '16 at 21:20

score 4 · Accepted Answer · edited May 23 '17 at 11:59

4

These two lines

  strcat(lexeme,lexeme1);
  strcat(lexeme,lexeme2);

are concatenating onto lexeme, which in turn points to uninitialised memory, allocated here:

  lexeme=(char *)malloc(sizeof(char) * 100);

To fix this either initialise the memory explicitly by calling:

  memset(lexeme, 0, 100);

or implicitly by using calloc() instead of malloc():

  lexeme = calloc(100, 1);

Update:

A 3rd option, as mentioned by Michi in his comment would be to replace the 1st call to strcat() by calling strcpy()

  strcpy(lexeme, lexeme1);
  strcat(lexeme, lexeme2);

This probably is the cheapest solution, at least in terms of speed.

In any case remove the useless cast to (char*) and replace sizeof char by 1 which it is by definition.

edited May 23 '17 at 11:59

Community

1
1

answered Mar 22 '16 at 19:48

alk

69,737
10
105
255

1

`strcpy` was missing there. – Michi Mar 22 '16 at 19:50
@Michi - if calloc is used to create the memory, then can a `strcat()` not be used as the very first copy? (i.e. without first calling `strcpy`?) – ryyker Mar 22 '16 at 20:11
1

@ryyker: Sure it can. – alk Mar 22 '16 at 20:13
@ryyker That's the whole point, `strcat` needs `strcpy` if you are going to use `malloc`, if you don't use `strcpy` then, `calloc` is needed, because of `malloc` which will not initialize it for you.. – Michi Mar 22 '16 at 20:15
`This probably is the cheapest solution, at least in terms of speed` I like this :)) – Michi Mar 22 '16 at 20:16
1

@Michi - I think we are in agreement. (it was a rhetorical question :) ) Thank you. – ryyker Mar 22 '16 at 20:21
1

@ryyker Of course :). Any way when `strcpy` is used the `'\0'` is copied to, which means that if [the program was written well](http://ideone.com/CKLCPE), will be no need of [calloc](http://ideone.com/qy1V1n). – Michi Mar 22 '16 at 20:35

ryyker · Answer 2 · 2016-03-22T20:02:39.997

1

As is shown by @Alk's answer (+1), the central problem is concatenating into uninitialized memory (needed to start with a creating memory, and initialization of lexeme). But here are some additional suggestions for avoiding memory creation/use issues:

Because sizeof(char); is always 1, malloc statement can be written:

char * dest=malloc((end-start+2));

But because malloc() does not initialize the memory it creates, and because of the nature of the problem you are seeing, I suggest two additional things:

1) check the values for source, start and end before using them in memory allocation statement.

2) use calloc() rather than malloc() as it will initialize all memory to a known value:

char * dest=calloc((end-start+2), 1);

edited Mar 22 '16 at 20:02

answered Mar 22 '16 at 19:48

ryyker

22,849
3
43
87

1

The problem is with `lexeme` – Michi Mar 22 '16 at 19:48
@Michi - _lesne_? I do not see it. – ryyker Mar 22 '16 at 19:50
1

`strcpy` was only thing that he needed. – Michi Mar 22 '16 at 19:50
1

@Michi - LOL, yes people tell me that. Thank you. – ryyker Mar 22 '16 at 19:52
1

The problem with `strcat` is, that they need `strcpy` too. So to fix it they use `calloc` which is not wrong, but not entirely what they need. – Michi Mar 22 '16 at 19:54
`use calloc() rather than malloc()` if the dest string has, lets say `calloc(100, 1)` will be a lot of `ZERO'S` there, if source has length of `15` – Michi Mar 22 '16 at 20:02
@Michi - Yes true, but if source only needs 15, then `char * string = calloc(15, 1);` will work. Also, since each location in memory is initialized with NULL terminator (using `calloc`), using `strcat` for very first call is OK. For `malloc` it would not be. – ryyker Mar 22 '16 at 20:18

Conditional jump or move error in valgrind

2 Answers2