0

I'm trying to dynamically allocate memory for (what is essentially) a 2-dimensional array of chars - i.e - an array of strings.

My code is as follows:

typedef char LineType[MAX_CHARS+1];
LineType* lines;

int c = 0;
int N = 2;

lines = (LineType *) malloc (N * sizeof( LineType) );
do { 
    if (c > N ) {
        N *=2;
        lines = (LineType*) realloc (lines, N * sizeof( LineType));
    }

    .
    .
    .
    c++;

} while ( . . . )

This compiles fine but fails at runtime, giving a warning about possible HEAP CORRUPTION and breaking at dbgheap.c (in : _CrtIsValidHeapPointer)

What am I doing wrong? I figured it's probably due to the mix of a fixed/dynamic dimensions in the data structure... But what is then the best way to declare and then dynamically allocate (and reallocate) memory for an array (of varying size) of strings (each of which is of a fixed size)?

Thanks a lot in advance

UPDATE 26/8/2012

I changed the code a bit to adjust it to people's comments and suggestions. The problem still persists...

trincot
  • 317,000
  • 35
  • 244
  • 286
Klayhamn
  • 25
  • 6

3 Answers3

2

Assuming c is used to index into lines, you need to test for c >= N, not c > N.

As an aside, I suggest using a typedef to make your code more readable. I would also avoid the redundant allocation code:

typedef char fixed_string[MAX_CHARS + 1];

int c = 0;
int N = 0;
fixed_string *lines = NULL;

do {
    if (c >= N ) {
        N = N ? 2*N : 2;
        lines = (fixed_string*) realloc (lines, N * sizeof(fixed_string));
    }
    ⋮
    c++;
} while (…);

As a further aside, be careful when using a growth factor of 2. It leaves behind holes that can never be reused by the same array. A factor of 1.5 (3*N/2) is safer.

EDIT: I note from other comments that you experience the crash at the point of reallocation. This is consistent with writing past the end of the array. A debug memory allocator will fill the space immediately surrounding an allocated block of memory with special bytes and check that those bytes are preserved the next time it does something with that block of memory. The HEAP CORRUPTION message signals that you have corrupted those surrounding bytes by writing outside the memory you were given.

Marcelo Cantos
  • 181,030
  • 38
  • 327
  • 365
  • Thanks, but I thought you can only realloc what you malloc/calloc first? Also, I didn't understand the part about the holes in the array - what do you mean? – Klayhamn Aug 25 '12 at 10:21
  • @Klayhamn: [realloc works like malloc](http://pubs.opengroup.org/onlinepubs/7908799/xsh/realloc.html) when passed a null pointer. – Marcelo Cantos Aug 25 '12 at 10:38
  • @Klayhamn: Each time you double your allocation, you leave behind a hole that is half the new size. The sum of all the holes left behind is guaranteed to be smaller than the new size, so you will never be able to reuse the holes. If, OTOH, you grow by 1.5, then, after four allocations, the three holes left behind amount to 0.75 + 0.75² + 0.75³ ≅ 1.73 times the current allocation, which means that the next allocation, which will only be 1.5 times the current allocation, may fit into the holes (depending on whatever else is going on in memory). – Marcelo Cantos Aug 25 '12 at 10:45
  • @Marcello Ah ok, I see.. thanks! But regarding your comment on the heap corruption: How could it be related to writing past the end of the array? I'm not writing anything yet, just allocating memory, and it still fails (exactly at the line of "realloc"). – Klayhamn Aug 26 '12 at 15:51
  • @Klayhamn: It isn't the allocation of the new block that's causing the error, it's the deallocation of the old block. – Marcelo Cantos Aug 26 '12 at 22:17
0

Make things more readable:

Instead of

char (*lines) [MAX_CHARS +1];

do a

typedef char LineType[MAX_CHARS+1];
LineType* lines;

In a similar fashion,

lines = (char (*) [MAX_CHARS +1]) calloc (N, sizeof( char (*) [MAX_CHARS +1]));
...
lines = (char (*) [MAX_CHARS + 1]) realloc (lines, N * sizeof( char (*) [MAX_CHARS +1]));

turns into

lines = malloc(N*sizeof(LineType));
...
lines = realloc(lines, N * sizeof(LineType));

Note: I replaced the calloc with malloc, simply because I never use calloc, so I'm not sure whether it tries to play alignment tricks.

Either way, a small typedef can improve readability a lot. Readable code is easier to get straight.

Christian Stieber
  • 9,954
  • 24
  • 23
0

This is hugely wrong

lines = (LineType *) malloc (N, sizeof( LineType *) );

as it allocates space for just a single pointer sizeof(LineType*) instead of a number of strings.

(It also happens to use the odd "comma operator" that allows you to have two expressions where only one is expected. It evaluates and discards what is on the left side, and keeps what is on its right side. Not very good here!)

A better allocation would be

lines = malloc(N * sizeof(LineType));

where we allocate space for N objects of type LineType.

There is a similar problem with the realloc, where you allocate space for pointers instead of whole objects.

Bo Persson
  • 90,663
  • 31
  • 146
  • 203
  • Thanks, I tried to follow Christian Stieber's advice and apparently failed to do so :) But I'm trying to understand what would happen when I indeed allocate for N*sizeof(LineType)... I mean, in the end, lines is a pointer... what happens if I do lines++? where does it jump to in the allocated memory exactly? lines[0]+MAX_CHARS+1 ? I ended up just defining lines as char **... but I'm still trying to understand how to solve it in the way I defined above :/ – Klayhamn Aug 26 '12 at 15:44