1

Here is my code which should dynamically allocate memory for storing strings:

#define _XOPEN_SOURCE 700

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

int main()
{
    char **path = NULL;

    path = (char **) malloc(sizeof(char *));

    for (int i = 0; i < 5; i++)
    {
            path[i] = strdup("TEST");
            path = (char **) realloc(path, sizeof(char *));
    }

    for (int i = 0; i < 5; i++)
    {
            printf("%s\n", path[i]);
    }

    return 0;
}

The above code breaks on the line where i have reallocated the memory.

According to me the the memory allocated to path on first malloc is as given below:

path -----> |   char *  | // path points to a character pointer which       inturn is pointing to nothing(Dangling).

So, at this point of time when the program ran the line:

path = (char **) malloc(sizeof(char *));

path[0] is currently in bounds and we can store starting address of one and only one string at path[0] and path[1] should be out of bounds at this point of time.

So, when the very first time we entered the for loop where we are storing the address of string at path[i] we will be able to that as:

path[0] = strdup("TEST"); // is in bounds.

To store another string we need some more memory that path should be pointing. So i did realloc in the line just below as:

path = (char **) realloc(path, (char *));

So, according to me path should look like this in memory as below:

path --------->|old char *|----->"TEST"
  |----------->|new char *|----->(Dangling) // Pointing Nowhere.

So, now path[1] is also in bounds and we should be able to use that memory location. So i can't figure out why i am getting this Segmentation fault when i run my code:

*** glibc detected *** ./dynamic: realloc(): invalid next size: 0x0000000000602010 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x75018)[0x7f743153e018]
/lib64/libc.so.6(+0x7ae96)[0x7f7431543e96]
/lib64/libc.so.6(realloc+0xfa)[0x7f74315441aa]
./dynamic[0x40067f]
/lib64/libc.so.6(__libc_start_main+0xe6)[0x7f74314e7bc6]
./dynamic[0x400569]
======= Memory map: ========
00400000-00401000 r-xp 00000000 00:27 14459340                           /home/xpansat/c/basic/dynamic
00600000-00601000 r--p 00000000 00:27 14459340                           /home/xpansat/c/basic/dynamic
00601000-00602000 rw-p 00001000 00:27 14459340                           /home/xpansat/c/basic/dynamic
00602000-00623000 rw-p 00000000 00:00 0                                  [heap]
7f742c000000-7f742c021000 rw-p 00000000 00:00 0 
7f742c021000-7f7430000000 ---p 00000000 00:00 0 
7f74312b2000-7f74312c8000 r-xp 00000000 fd:01 173                        /lib64/libgcc_s.so.1
7f74312c8000-7f74314c7000 ---p 00016000 fd:01 173                        /lib64/libgcc_s.so.1
7f74314c7000-7f74314c8000 r--p 00015000 fd:01 173                        /lib64/libgcc_s.so.1
7f74314c8000-7f74314c9000 rw-p 00016000 fd:01 173                        /lib64/libgcc_s.so.1
7f74314c9000-7f743161d000 r-xp 00000000 fd:01 27                         /lib64/libc-2.11.1.so
7f743161d000-7f743181d000 ---p 00154000 fd:01 27                         /lib64/libc-2.11.1.so
7f743181d000-7f7431821000 r--p 00154000 fd:01 27                         /lib64/libc-2.11.1.so
7f7431821000-7f7431822000 rw-p 00158000 fd:01 27                         /lib64/libc-2.11.1.so
7f7431822000-7f7431827000 rw-p 00000000 00:00 0 
7f7431827000-7f7431846000 r-xp 00000000 fd:01 20                         /lib64/ld-2.11.1.so
7f7431a14000-7f7431a17000 rw-p 00000000 00:00 0 
7f7431a44000-7f7431a45000 rw-p 00000000 00:00 0 
7f7431a45000-7f7431a46000 r--p 0001e000 fd:01 20                         /lib64/ld-2.11.1.so
7f7431a46000-7f7431a47000 rw-p 0001f000 fd:01 20                         /lib64/ld-2.11.1.so
7f7431a47000-7f7431a48000 rw-p 00000000 00:00 0 
7fff62f8c000-7fff62fa2000 rw-p 00000000 00:00 0                          [stack]
7fff62fff000-7fff63000000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]

I can't figure what i am doing wrong, because i was able to run my code if i changed my realloc line as below:

path = (char **) realloc(sizeof(char *)*8);

If is did that multiplication by 8 my code runs but i want to allocate exact memory i want, Any ideas why it is not working without that multiply by 8.

chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
mSatyam
  • 531
  • 7
  • 25
  • You have buffer overrun somewhere. You appear to be on Linux, so just run it under [Valgrind](http://www.valgrind.org) to find it. – Jan Hudec Jul 02 '13 at 12:22
  • 2
    `path = (char **) malloc(sizeof(char *));`, then `path = (char **) realloc(path, sizeof(char *));` <- accessing `path[1]` in `for (int i = 0; i < 5; i++)` is undefined behaviour. You trample all over memory you have no right to access. – Daniel Fischer Jul 02 '13 at 12:26
  • Note, that the `realloc` is totally pointless, because you request exactly the same size you allocated the array to initially. Remember, `sizeof(type)` is a compile-time constant! – Jan Hudec Jul 02 '13 at 12:28
  • 1
    Casting `malloc()/calloc()/realloc()` in C is not needed, even more it **not recommended**: http://stackoverflow.com/q/605845/694576 – alk Jul 02 '13 at 12:41

2 Answers2

5

Here

path = (char **) malloc(sizeof(char *));

you are allocating a pointer to a pointer to char, that is space for one pointer to a C-"string".

Then here:

for (int i = 0; i < 5; i++)
{
   path[i] = strdup("TEST");
   path = (char **) realloc(path, sizeof(char *));
}

you not only use one pointer to a C-"string" but reallocation for more does nothing more then just reallocate the same size as before, but infact you access as if you added memory for one more pointer in each iteration.

Doing so you overwrite memory not allocated (or alrerady in use by something else) and mess up the programs memory management.

A somewhat more straight forward approach to this would be:

  ...

  char ** path = NULL;

  for (size_t i = 0; i < 5; ++i)
  {
    path = realloc(path, (i+1) * sizeof(*path));
    path[i] = strdup("TEST");
  }

  ...
alk
  • 69,737
  • 10
  • 105
  • 255
  • hello, actually I don't understand what did the OP want to do with this code but when I include a declaration for `strdup` the code works, i.e test is printed 5 times.Can you please explain it for me.thanks! – 0decimal0 Jul 02 '13 at 13:07
  • 1
    @PHIfounder C allows you do stuff that causes undefined behavior, like the original code here. What happens in such cases can be very random, it could be a catastrophic failure, or it could appear to work fine, atleast today. – nos Jul 02 '13 at 13:12
  • 1
    @PHIfounder: You might like to run the program compiled from OP's orginal source using Valgrind (http://valgrind.org) and you'll see what is going wrong behind the scene, even if the program might not crash. – alk Jul 02 '13 at 13:13
  • Does Valgrind run on windows? I use gdb. – 0decimal0 Jul 02 '13 at 13:17
  • 1
    @PHIfounder: Do you use MinGW or Cygwin? For MinGW this might help: http://sourceforge.net/p/valgrind4win/wiki/DevelopmentEnvironment/ – alk Jul 02 '13 at 13:20
  • MinGW ,thank you very much for the link.An upvote for you. – 0decimal0 Jul 02 '13 at 13:27
4

Following your style, change

path = (char **) realloc(path, sizeof(char *));

to

path = realloc(path, (i+2) * sizeof(char *));

This allocates for the next loop. Should you want to reallocate per each loop, it makes more sense to allocate beforehand as in:

path = realloc(path, (i+1) * sizeof(char *));
path[i] = strdup("TEST");

OR

As other say, allocate all your memory up front with path = malloc(5 * sizeof(char *)). Dropping the cast of (char **) is the preferred coding practice.

OR

Allocate all your memory up front with

path = calloc(5, sizeof(char *));

This have 2 benefits of calloc(num,size) over malloc(size). The memory is zero filled. Should the multiplication of the num * size overflow (obviously not in this case), the calloc() still would correctly handle it.


Another coding practice thought: using ptr = malloc(sizeof(*ptr)) has a nice advantage over ptr = malloc(sizeof(char *)). Should the type of *ptr change (even though not likely here), no coding change would be needed in various malloc() calls. (Applies to realloc() and calloc(), too.)

chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
  • 1
    This is probably what the code was supposed to mean, but it would deserve some explanation. – Jan Hudec Jul 02 '13 at 12:29
  • Needs `i+2`, after accessing `path[i]`, `path` must already point to at least `i+1` items, or you have UB. – Daniel Fischer Jul 02 '13 at 12:30
  • @Daniel Fischer Nice catch on the +2. – chux - Reinstate Monica Jul 02 '13 at 12:44
  • @DanielFischer agreed with you after running the code but can't understand why i+2 is necessary and can't get what is UB. – mSatyam Jul 02 '13 at 17:34
  • The second time in the OP's `for` loop, the assignment `path[1] = strdup("TEST");` occurs. By this time, `path` needs to be _2_ pointers big. Thus the realloc() of the previous loop, when `i` was 0, must provide for 2. In the last & 5th time through the loop, when i = 4, realloc() would make for 6. path[6] would never be used. Sooooo, do the reallloc() _before_ the assignment, rather than _afterward_. – chux - Reinstate Monica Jul 02 '13 at 17:57
  • One more thing i need to ask, suppose if i allocated 4bytes of memory using malloc and then later i did realloc by passing in the base address of the memory allocated earlier using malloc and in realloc i specified to allocate 8 bytes of memory. So, now in total i will have 12 bytes of memory or 8 bytes of memory. – mSatyam Jul 05 '13 at 03:50
  • 1
    @msatyam 8 bytes. The first 4 are released in the `realloc ()` call. – chux - Reinstate Monica Jul 07 '13 at 22:40