views:

254

answers:

3

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.

+5  A: 

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.

Evan Teran
wait guys, this is a simplified version of the code.I've narrowed it down to the minimal to reproduce the error.I know it's currently does noting useful... expect for reproducing the problem
Maxim Veksler
you asked why **that** code leaks, you have 3 `malloc` calls and zero `free` calls. That is why...
Evan Teran
OK AGAIN: This code is here only to show how the problem can be reproduced. It is in development and was stripped down to the MINIMUM REQUIRED TO DEMONSTRATE THE PROBLEM.Please leave all other side notes (which are very correct but irrelevant) aside
Maxim Veksler
@Evan: I have edited to topic. Sorry for the confusion.
Maxim Veksler
@Evan: Thank you, now I understand my problem. The buffer overflow was the issue which I missed. Thank you :) Note the "student-question" question tag.
Maxim Veksler
no problem, everyone is a beginner at sometime.
Evan Teran
A: 

In the function "aggregatedArray" you are not assigning the pointers returned from malloc to a variable so that they can be freed later on. They are lost in space!

+1  A: 

The following statement allocates a memory of 18 bytes ("3 5 234 11 77 44 5")

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

but you are putting integers onto that memory area ... so it wont last long when you would have usedup all space...so thats definitely wrong to do.

further.. you are not using up any free() calls.. so that would be a problem too..

ashishsony
oops... Evan Teran had already responded similar but better..
ashishsony