views:

224

answers:

4

I have a heap allocation error that I cant spot in my code that is picked up on vanguard/gdb on Linux but runs perfectly on a Windows cygwin environment. I understand that Linux could be tighter with its heap allocation than Windows but I would really like to have a response that discovers the issue/possible fix. I'm also aware that I shouldn't typecast malloc in C but it's a force of habit and doesn't change my problem from happening. My program actually compiles without error on both Linux & Windows but when I run it in Linux I get a scary looking result:

malloc.c:3074: sYSMALLOc: Assertion `(old_top == (((mbinptr) (((char *) &((av)->bins[((1) - 1) * 2])) - __builtin_offsetof (struct malloc_chunk, fd)))) && old_size == 0) || ((unsigned long) (old_size) >= (unsigned long)((((__builtin_offsetof (struct malloc_chunk, fd_nextsize))+((2 * (sizeof(size_t))) - 1)) & ~((2 * (sizeof(size_t))) - 1))) && ((old_top)->size & 0x1) && ((unsigned long)old_end & pagemask) == 0)' failed. Aborted

Attached snippet from my code that is being pointed to as the error for review:

/* Main */

int main(int argc, char * argv[]) {

    FILE *pFile;  
    unsigned char *buffer;  
    long int lSize;  

    pFile = fopen ( argv[1] , "r" );
    if (pFile==NULL) {fputs ("File error on arg[1]",stderr); return 1;}

    fseek (pFile , 0 , SEEK_END);
    lSize = ftell (pFile);
    rewind (pFile);

    buffer = (char*) malloc(sizeof(char) * lSize+1);
    if (buffer == NULL) {fputs ("Memory error",stderr); return 2;}

    bitpair * ppairs = (bitpair *) malloc(sizeof(bitpair) * (lSize+1));

    //line 51 below
    calcpair(ppairs, (lSize+1));

    /* irrelevant stuff */

    fclose(pFile);
    free(buffer);
    free(ppairs);  
}

typedef struct {  
long unsigned int a;  //not actual variable names...  Yes I need them to be long unsigned  
long unsigned int b;  
long unsigned int c;  
long unsigned int d;  
long unsigned int e;  
} bitpair;  

void calcpair(bitpair * ppairs, long int bits);

void calcPairs(bitpair * ppairs, long int bits) {

    long int i, top, bot, var_1, var_2;
    int count = 0;

    for(i = 0; i < bits; i++) {

        top = 0;

        ppairs[top].e = 1;

        do {
            bot = count;
            count++;
        } while(ppairs[bot].e != 0);

        ppairs[bot].e = 1;

        var_1 = bot;
        var_2 = top;

        bitpair * bp = &ppairs[var_2];
        bp->a = var_2;
        bp->b = var_1;
        bp->c = i;

        bp = &ppairs[var_1];
        bp->a = var_2;
        bp->b = var_1;
        bp->c = i;

    }

    return;
}

gdb reports: free(): invalid pointer: 0x0000000000603290 *

valgrind reports the following message 5 times before exiting due to "VALGRIND INTERNAL ERROR" signal 11 (SIGSEGV):
Invalid read of size 8
==2727== at 0x401043: calcPairs (in /home/user/Documents/5-3/ubuntu test/main)
==2727== by 0x400C9A: main (main.c:51)
==2727== Address 0x5a607a0 is not stack'd, malloc'd or (recently) free'd

+1  A: 

At a wild guess ftell is returning -1 and malloc doesn't like being asked to allocate zero bytes. The behaviour of malloc(0) is implementation dependent in C.

anon
I can confirm through test that it is lSize is being handed the proper size, 27 in this case (sometimes much much larger, hence the need for long).
Shawn
It's implementation-dependent, but it has to return a pointer value that you can safely pass to `free()` (including `NULL`). It's not allowed to abort.
caf
A: 

Your second call to malloc never has its return value checked. Modify it so it looks more like the first one, as in:

bitpair * ppairs = (bitpair *) malloc(sizeof(bitpair) * (lSize+1));
if (ppairs == NULL) {fputs ("Memory error",stderr); free(buffer); return 3;}

Also, remember malloc expects a size_t (the definition of which is implementation-dependent) for an argument. Make sure that when you pass (sizeof(bitpair) * (lSize+1)) to malloc, you are not overflowing a size_t (if size_t is defined as unsigned int, you could run into problems since lSize is a long).

bta
Thanks, I did forget to do that. Unfortunately that the malloc issue remains
Shawn
Changing lSize and associated variables in function to regular int resulted in the same run-time error
Shawn
Random thoughts (may or may not change anything): Instead of declaring `ppairs` in the same line where you call `malloc`, declare it at the top of the function. Also, try moving the definition of `bitpair` up above your `main` function.
bta
+1  A: 

It looks like you are expecting malloc to return pre-zeroed memory.

    do {
        bot = count;
        count++;
    } while(ppairs[bot].e != 0);

could easily get to the end of your ppairs without finding a zeroed ppairs[bot].e

You want to use calloc instead of malloc, that clears the memory before returning it.

bitpair * ppairs = (bitpair *) calloc(sizeof(bitpair) * (lSize+1));
aspo
This helped although you didn't call calloc correctly (needs 2 arguments): void * calloc ( size_t num, size_t size ); When I use calloc with lSize+1 as num and the argument above as the size I can run against small target files, but larger ones (40KB) generate the Memory Error from if(ppairs==NULL).
Shawn
Possibly easier than using `calloc` is keeping your current `malloc` calls and using `memset` to zero them out after they are allocated.
bta
I dont have experience with memset but doing: memset (ppairs, 0, lSize+1); along with malloc on ppairs compiles but still gives the same heap error
Shawn
memset (ppairs, 0, sizeof(bitpair) * (lSize+1));
aspo
@aspo- Still gives the malloc issue at run-time
Shawn
@Shawn - but I suspect it fixes the Valgrind error you were seeing. Judging from the mistakes in the code you've shown, I suspect you are writing to unmalloced memory somewhere else in the "irrelevant stuff". That can end up overwriting data structures that malloc uses which will cause badness later on.
aspo
+1  A: 

Looks Like An Array Overrun

Nothing is keeping this loop from overrunning the end of the ppair array:

    do { 
        bot = count; 
        count++; 
    } while(ppairs[bot].e != 0); 

Especially since this line will overwrite your terminating zero:

ppairs[bot].e = 1;

Try this instead:

    do { 
        bot = count; 
        count++; 
    } while((bot < bits) && (ppairs[bot].e != 0)); 

Yours, Tom

Tom DeGisi