summary: malloc.c:3074 - Why does this code causes the error

Question

The attached below C code when run gives the error

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

On each any every call for malloc(21); (See below). Can someone please explain why?? I've tried every possible thing I can think of and it still fails.

File: summary.c

/* 
* File:   summary.c
* Author: Maxim Veksler
*
* Created on December 4, 2009, 3:09 AM
*/

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

/*
* Main for Maman 12, task 1 : Array summary utility
*/
int main(int argc, char** argv) {
    /*STUB*/
    char str[100];
    strcpy(str, "3 5 234 11 77 44 5");
    /*STUB*/

    int resultsSize;
    int* results;
    int* aggregated;

    results = parseInput(str, &resultsSize);
    aggregatedArray((int*)NULL, (int)NULL);


    return 0;
}

File manipulation.c

    /*
    * File:   manipulation.c
    * Author: Maxim Veksler
    *
    * Created on December 4, 2009, 3:09 AM
    */

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

    /*
    * Parse the input from user, dynamically allocate memory to the maximum
    * possible requirment. Then convert the array of string tokens into an
    * simple array of integers.
    */
    int* parseInput(char* input, int* nOfResults) {
        /* Allocate memory by the maximum possibly required size for int... */
        int *results = (int*) malloc(strlen(input));

        int* insertIterator = results;
        char* pch;


        /* We trash the user input, but it's cool - Worthless as usual. */
        pch = strtok(input,"\t ,.-");
        *nOfResults = 0;

        while(pch != NULL) {
        (*nOfResults)++;

        *insertIterator = atoi(pch);
        insertIterator++;
        pch = strtok(NULL, "\t ,.-");
        }

        return results;
    }


    /*
    * Summary the values given in the int array and return adress to new array
    * containing an increasin sum of the values.
    */
    int* aggregatedArray(int* inputArray, int size) {
        int* results;
        malloc(20);
        malloc(21);
    }

EDIT Please take into account this code is a stripped down version that is brought here show the problem. I've deleted all the non relevant parts.

Answer 1

EDIT: woah, I just realized that you have a pretty bad logic error in your code. This isn't just leaking, you have a buffer overflow too!

int *results = (int*) malloc(strlen(input));

That will allocate 18 bytes (the lengh of input) and treat that like an array of int's, which means you can fit 18 / sizeof(int) ints in it. assuming usual x86 size, that means you can only fit (18 / 4) == 4.5 integers! Later your code will write several more than that into the array. Big error.

To fix the issue, you should be using realloc. Something like this:

int *results = malloc(sizeof(int));
int result_size = 1;
int result_count = 0;

while(/*whatever*/) {
    /* ok, i want to add an element to results */
    if(result_count == result_size - 1) {
        int *p = realloc(results, (result_size + 1) * sizeof(int));
        if(!p) {
            free(results);
            return NULL; /* no more memory! */
        }
        results = p;
        result_size++;
    }
    results[result_count++] = /*value*/
}
return results;

---

It leaks because you have 2 mallocs which you don't store the result of anywhere. This makes it impossible to free the pointer those calls return.

In fact, I'm not sure what aggregatedArray is supposed to be actually doing, at the moment, it does nothing but leak.

Also, you have results = parseInput(str, &resultsSize); where parseInput returns a malloced pointer. You should have a free(results); later on when you no longer need this (probably right after the aggregatedArray call).

Finally, as a side note. I would guess that aggregatedArray((int*)NULL, (int)NULL); should in fact be aggregatedArray(results, resultsSize); :-P.