Variable Arguments in C creating error in Valgrind

Question

I was trying to run a program which uses a function concat_str. It can take in multiple arguments as strings and the end of arguments is denoted by "quit". The code for my function is given below:

char *concat_str(char *str1, ...)
{
    va_list pstr;
    char *minion = NULL, *temp = NULL;
    minion = (char*) malloc (sizeof(str1));
    strcpy (minion,str1);
    va_start (pstr, str1);
    if ( strcmp ("quit",str1) == 0)
    {
        va_end (pstr);
        return minion;
    }
    while (1)
    {
        temp = va_arg (pstr, char *);
        if ( strcmp ("quit", temp) == 0)
        {
            break;
        }
        minion = (char*) realloc (minion, sizeof(temp));
        strncat (minion,temp,sizeof(temp));
    }
    va_end (pstr);
    return minion;
}

A calling statement for the same would be:

char *result;
result = concat_str("hello", "hai", "how", "are", "you", "quit");

I do get the correct output. But when I ran it on valgrind with memcheck tool, I got many errors. The error is as below:

==2635== Invalid write of size 1
==2635==    at 0x4A065D3: strncat (mc_replace_strmem.c:218)
==2635==    by 0x400A7E: concat_str (var_fun.c:23)
==2635==    by 0x400757: main (var_main.c:15)
==2635==  Address 0x4C33038 is 0 bytes after a block of size 8 alloc'd
==2635==    at 0x4A0590B: realloc (vg_replace_malloc.c:306)
==2635==    by 0x400A5F: concat_str (var_fun.c:22)
==2635==    by 0x400757: main (var_main.c:15)
==2635==
==2635== Invalid read of size 1
==2635==    at 0x4A06594: strncat (mc_replace_strmem.c:218)
==2635==    by 0x400A7E: concat_str (var_fun.c:23)
==2635==    by 0x400757: main (var_main.c:15)
==2635==  Address 0x4C33038 is 0 bytes after a block of size 8 alloc'd
==2635==    at 0x4A0590B: realloc (vg_replace_malloc.c:306)
==2635==    by 0x400A5F: concat_str (var_fun.c:22)
==2635==    by 0x400757: main (var_main.c:15)
==2635==
==2635== Invalid write of size 1
==2635==    at 0x4A065BF: strncat (mc_replace_strmem.c:218)
==2635==    by 0x400A7E: concat_str (var_fun.c:23)
==2635==    by 0x400757: main (var_main.c:15)
==2635==  Address 0x4C33038 is 0 bytes after a block of size 8 alloc'd
==2635==    at 0x4A0590B: realloc (vg_replace_malloc.c:306)
==2635==    by 0x400A5F: concat_str (var_fun.c:22)
==2635==    by 0x400757: main (var_main.c:15)

==2635==
==2635== Invalid read of size 1
==2635==    at 0x4A066D4: strlen (mc_replace_strmem.c:246)
==2635==    by 0x32DAC46B18: vfprintf (in /lib64/libc-2.5.so)
==2635==    by 0x32DAC4D3A9: printf (in /lib64/libc-2.5.so)
==2635==    by 0x40076E: main (var_main.c:16)
==2635==  Address 0x4C33038 is 0 bytes after a block of size 8 alloc'd
==2635==    at 0x4A0590B: realloc (vg_replace_malloc.c:306)
==2635==    by 0x400A5F: concat_str (var_fun.c:22)
==2635==    by 0x400757: main (var_main.c:15)
==2635==
==2635== Invalid read of size 1
==2635==    at 0x32DAC6CE09: _IO_file_xsputn@@GLIBC_2.2.5 (in /lib64/libc-2.5.so)
==2635==    by 0x32DAC464B2: vfprintf (in /lib64/libc-2.5.so)
==2635==    by 0x32DAC4D3A9: printf (in /lib64/libc-2.5.so)
==2635==    by 0x40076E: main (var_main.c:16)
==2635==  Address 0x4C33040 is 8 bytes after a block of size 8 alloc'd
==2635==    at 0x4A0590B: realloc (vg_replace_malloc.c:306)
==2635==    by 0x400A5F: concat_str (var_fun.c:22)
==2635==    by 0x400757: main (var_main.c:15)
==2635==
==2635== Invalid read of size 1
==2635==    at 0x32DAC6CE1C: _IO_file_xsputn@@GLIBC_2.2.5 (in /lib64/libc-2.5.so)
==2635==    by 0x32DAC464B2: vfprintf (in /lib64/libc-2.5.so)
==2635==    by 0x32DAC4D3A9: printf (in /lib64/libc-2.5.so)
==2635==    by 0x40076E: main (var_main.c:16)
==2635==  Address 0x4C3303F is 7 bytes after a block of size 8 alloc'd
==2635==    at 0x4A0590B: realloc (vg_replace_malloc.c:306)
==2635==    by 0x400A5F: concat_str (var_fun.c:22)
==2635==    by 0x400757: main (var_main.c:15)
==2635==
==2635== Invalid read of size 1
==2635==    at 0x32DAC6CD66: _IO_file_xsputn@@GLIBC_2.2.5 (in /lib64/libc-2.5.so)
==2635==    by 0x32DAC464B2: vfprintf (in /lib64/libc-2.5.so)
==2635==    by 0x32DAC4D3A9: printf (in /lib64/libc-2.5.so)
==2635==    by 0x40076E: main (var_main.c:16)
==2635==  Address 0x4C33038 is 0 bytes after a block of size 8 alloc'd
==2635==    at 0x4A0590B: realloc (vg_replace_malloc.c:306)
==2635==    by 0x400A5F: concat_str (var_fun.c:22)
==2635==    by 0x400757: main (var_main.c:15)
==2635==
==2635== Invalid read of size 1
==2635==    at 0x32DAC6CD7A: _IO_file_xsputn@@GLIBC_2.2.5 (in /lib64/libc-2.5.so)
==2635==    by 0x32DAC464B2: vfprintf (in /lib64/libc-2.5.so)
==2635==    by 0x32DAC4D3A9: printf (in /lib64/libc-2.5.so)
==2635==    by 0x40076E: main (var_main.c:16)
==2635==  Address 0x4C33039 is 1 bytes after a block of size 8 alloc'd
==2635==    at 0x4A0590B: realloc (vg_replace_malloc.c:306)
==2635==    by 0x400A5F: concat_str (var_fun.c:22)
==2635==    by 0x400757: main (var_main.c:15)

Source of error (my own deduction):

Line 22 : minion = (char*) realloc (minion, sizeof(temp));

Realloc returns the address of a pointer of a new block to minion. But the old block creates problems.

Things I tried:

I changed strncat (minion,temp,sizeof(temp)); to strncat (minion,temp,sizeof(temp) + 10); . This reduced some errors. But if the string arguments were lengthy, I again got the same errors. By the by, I didn't understand how this solved the issue.
I changed minion = (char*) realloc (minion, sizeof(temp)); to

char t = NULL; t = minion; minion = (char) realloc (t, sizeof(temp)); free (t);

Please tell me, if my source of error is correct and suggest what I should do to solve this issue.

Note: Don't paste line numbers with code. Also For `minion = (char*) malloc (sizeof(str1));` Do not cast result of malloc. And here you need `strlen` not `sizeof`. — Jayesh Bhoi, Sep 18 '14 at 12:37
Don't assign the result of `realloc` to the same pointer you reallocate. Think of what will happen if `realloc` fails and returns `NULL`, then you will loose the original pointer and have a memory leak (besides the fact that you don't check for failure at all). — Some programmer dude, Sep 18 '14 at 12:43
@JoachimPileborg. Please read the things i tried , second point. — 0aslam0, Sep 18 '14 at 12:45
@JoachimPileborg I am actually calling free (result) in my main function — 0aslam0, Sep 18 '14 at 12:46
It doesn't matter if you call `free` because it any of the allocation functions fail you will have a `NULL` pointer. And you should do the reallocation/assignment in the opposite order, i.e. assign the `realloc` result to `t` and then if it's not `NULL` assign `t` to `minion`. And you still [should not cast the result of `malloc` (and family)](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). — Some programmer dude, Sep 18 '14 at 12:48

M Oehm · Accepted Answer · 2014-09-18T12:44:51.153

5

The expression

sizeof(str1)

will yield the size of a pointer, not the string length. You should allocate

minion = malloc(strlen(str1) + 1);

For your reallocations, you must provide the size of the whole array, not only the newly allocated storage, so you should keep track of your string length.

And finally, a stylistic hint: It is common to use NULL as a sentinel value to terminate string list instead of using an arbitrary literal like your "quit".

edited Sep 18 '14 at 12:44

answered Sep 18 '14 at 12:38

M Oehm

28,726
3
31
42

len = strlen (minion); minion = realloc (minion, len + strlen (temp)); Is this your suggestion. Getting new errors.. – 0aslam0 Sep 18 '14 at 12:56
@xachu4u: Don't forget the space for the terminating `'\0'`, which `strlen` will not take into account. There's also no need to re-calculate the string length with `strlen` every time. – M Oehm Sep 18 '14 at 12:59

score 0 · Answer 2 · answered Sep 18 '14 at 12:40

0

minion = (char*) malloc (sizeof(str1));

Here you need strlen not sizeof. Because char *str1 is char pointer and sizeof pointer should be 4 or 8 bytes according machine configuration.

So it will

minion =  malloc (strlen(str1)+1); //No need to cast result of malloc

answered Sep 18 '14 at 12:40

Jayesh Bhoi

24,694
15
58
73

what about inside the loop body? – 0aslam0 Sep 18 '14 at 12:58

score 0 · Answer 3 · answered Sep 18 '14 at 13:10

Finally this code worked for me. I removed the type cast (char*) , which was unnecessary. I changed sizeof to strlen wherever required.

Thank you Jayesh, Joachim and M ohem for your quick replies.

char *concat_str(char *str1, ...)
   {
    va_list pstr;
    char *minion = NULL, *temp = NULL;
    int len;
    minion = malloc (strlen(str1) + 1);
    strcpy (minion,str1);
   va_start (pstr, str1);
   if ( strcmp ("quit",str1) == 0)
          {
           va_end (pstr);
           return minion;
          }
   while (1)
          {
           temp = va_arg (pstr, char *);
           if ( strcmp ("quit", temp) == 0)
                  {
                   break;
                  }
           len = strlen (minion);
           minion = realloc (minion, len + strlen (temp) + 2);
           strncat (minion, temp, strlen(temp) + 1);
          }
   va_end (pstr);
   return minion;
  }

~

Variable Arguments in C creating error in Valgrind

3 Answers3