2

My goal is to make reverse two digits like 123456 to 563412. I'm using valgrind tool to check memory leakage problem but strlen(reverse_chr) function makes this error:

Conditional jump or move depends on uninitialized value(s)

Here is my code:

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

int main()
{
    char chr[] = "123456";
    char* reverse_chr=(char *) malloc(strlen(chr)+1);
    memset(reverse_chr, 0, strlen(chr));
    int chrlen=strlen(chr);

    for (int t=0; t<chrlen; t+=2)
    {
        reverse_chr[t]=chr[chrlen-t-2];
        reverse_chr[t+1]=chr[chrlen-t-1];
    }
    int len_reverse_chr = strlen(reverse_chr);
    free(reverse_chr);
    return 0;
}

I expect output without any valgrind error.

Rushit Solanki
  • 25
  • 6
  • Since you're using Valgrind and are concerned about coding practices, remember, [don't cast the result of `malloc`](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). It's also worth noting you call `strlen` four times, once inside a loop(!), when each time it's going to return exactly the same result. Call it once, save the result, and use that result four times. – tadman Apr 24 '19 at 04:49
  • 3
    What line in your code does it complain about? Please add a comment about it in the code you show. Also please don't cut things like `#include` or other things needed to make your example build. And of course always include the actual output (in full, complete, and copy-pasted without modifications) in your question. Please read about [how to ask good questions](http://stackoverflow.com/help/how-to-ask), as well as [this question checklist](https://codeblog.jonskeet.uk/2012/11/24/stack-overflow-question-checklist/). – Some programmer dude Apr 24 '19 at 04:50
  • It seems like you should be using `++t` instead of `t+=2`. – tadman Apr 24 '19 at 04:51
  • 2
    I also suggest you take some time to think about the logic of your loop, what it does, what character is placed at `chr[strlen(chr) - 0]` (which happens at the first iteration of the loop), and what happens to the elements of `reversed_chr` that you skip over with `t += 2`. – Some programmer dude Apr 24 '19 at 04:52
  • 1
    Lastly, this might be a very good time to [learn how to debug your programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). – Some programmer dude Apr 24 '19 at 04:55
  • 1
    Now the code is drastically different, different enough to really warrant a new question IMO, as it makes earlier comments obsolete and factually wrong. And you *still* need to learn how to debug your application. Even some simple [rubber duck debugging](https://en.wikipedia.org/wiki/Rubber_duck_debugging) should help you figure out what might be wrong. – Some programmer dude Apr 24 '19 at 05:19
  • Oh, and you should probably explain why you copy two characters at a time in your loop. There's no reason for it and it will definitely break on strings with odd lengths (which could be the reason behind your Valgrind error). – Some programmer dude Apr 24 '19 at 05:24
  • Lastly (again), there's no `` header file in C. But there is in ***C++***... So are you really programming in C? And talking about header files, read the first comment by @tadman carefully, follow the link in it, read that carefully. – Some programmer dude Apr 24 '19 at 05:25
  • `malloc` / `memset` could be replaced by `calloc`. – melpomene Apr 24 '19 at 06:24
  • What's the full valgrind output? – melpomene Apr 24 '19 at 06:26
  • I added **1 more byte** to malloc then it works fine without error. `char* reverse_chr=(char *) malloc(strlen(chr)+2);` `memset(reverse_chr, 0, strlen(chr)+1);` – Rushit Solanki Apr 24 '19 at 06:52
  • @RushitSolanki The malloc part is irrelevant. What fixed it was the change to `memset`. – melpomene Apr 24 '19 at 06:58

1 Answers1

1

The problem is that reverse_chr is not a valid string as it's not properly terminated.

char* reverse_chr=(char *) malloc(strlen(chr)+1);
memset(reverse_chr, 0, strlen(chr));

You allocate 7 bytes, but only set the first 6 to 0.

for (int t=0; t<chrlen; t+=2)
{
    reverse_chr[t]=...
    reverse_chr[t+1]=...

This for loop also only writes to the first 6 elements of reverse_chr.

int len_reverse_chr = strlen(reverse_chr);

Then this line tries to find a NUL byte in reverse_chr, but the first 6 elements aren't '\0' and the 7th is uninitialized (hence the complaint by valgrind).

Fix:

Either do

reverse_chr[chrlen] = '\0';

after the loop, or use calloc:

reverse_chr = static_cast<char *>(calloc(strlen(chr)+1, sizeof *reverse_chr));

This way all allocated bytes are initialized (and you don't need memset anymore).

melpomene
  • 84,125
  • 8
  • 85
  • 148