CODEWITHSUNDEEP
cmemoryheap-memoryrealloccorruption
Klayhamn
Klayhamn

Reputation: 35

realloc fails (in C) for a pointer to an array

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...

Upvotes: 0

Views: 1777

Answers (3)

Bo Persson
Bo Persson

Reputation: 92211

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.

Upvotes: 0

Marcelo Cantos
Marcelo Cantos

Reputation: 185852

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.

Upvotes: 2

Christian Stieber
Christian Stieber

Reputation: 12496

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.

Upvotes: 0

Related Questions