Dynamic array inside struct and malloc fail

Question

Consider the following abstracted code that reads some bytes from a file:

typedef struct A{
int size;
char * dataArray;
}A

A load(char* filename, int inSize)
{
    A newA;
    newA.size = inSize;
    FILE *filePtr;
    filePtr = fopen(filename,"rb");

    char buff[1];
    int i = 0;

    newA.dataArray = ( char*)malloc(sizeof(char) * newA.size);
    for (i = 0; i < newA.size; i++)
    {
        fread(buff, sizeof(char), 1, filePtr);
        newA.dataArray[i] = buff[0];
    }

    char* copyOfDataArray = (char*)malloc(sizeof(char) * newA.size);

    for (i = 0; i < newA.size; i++)
    {
        fread(buff, sizeof(char), 1, filePtr);
        copyOfDataArray[i] = newA.dataArray[i];
    }

    newA.dataArray = copyOfDataArray;
    return newA
}

void Initialize()
{
    A first = load("file1", 100);
    A second = load("file2", 20);
}

Both calls to function load return the expected result (data array has the same bytes as the file). Variables first and second are never used again.

However after a couple of hundreds lines of code the program always crashes with:

*malloc.c:2451: sYSMALLOC: Assertion '(old_top == (..... failed.*

The crash always occurs on the same line of code, but that line has nothing to do with variables first, second or even with struct A whatsoever.

My question is: is my way of instancing and loading 'first' and 'second' wrong? Can it cause some kind of memory leak / memory overflow that crashes the program long after the load function has finished?

Bonus: The crash does not occur if I only load "file1", as soon as i load both "file1" and "file2" the crash reappears.

Sorry for the long question.

What is `size`? Why are you reading from the file in the second loop, you just discard the data? Why read one byte at a time? Why do you overwrite the `newA.dataArray` pointer (and get a memory leak)? — Some programmer dude, Sep 05 '14 at 12:48
Oh, and in C you [should not cast the result of `malloc`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). — Some programmer dude, Sep 05 '14 at 12:48
Are you freeing the allocated memory when leaving the function? — Sathish, Sep 05 '14 at 12:56
As for your assertion error, are you sure you're not writing beyond the bounds of the allocated memory anywhere? Try using tools such as [Valgrind](http://valgrind.org/) to find memory access problems. — Some programmer dude, Sep 05 '14 at 13:00
@JoachimPileborg size is a number that is guaranteed to be smaller than the number of reads from the file. This I can be sure of. I disregard the data because this example is extremely abstracted - in reality I do change some bytes, but the number of bytes is the same in the original and new dataArray. I did not know that I am not allowed to overwrite newA.dataArray. I will check the link agains malloc result casting. — Nitkov, Sep 05 '14 at 13:09
What is `size`? You're reading `size` bytes from the file into your malloc'd memory, but that used `newA.size = inSize` - I can't see anything called `size`, so if this code compiles, you must have a global somewhere. Please post the _complete and self-contained_ code you actually compile and run. Or, at the very least, check where `size` is coming from and what value it has. — Useless, Sep 05 '14 at 13:13
Edited mistakes - in the real program i use newA.size, not size. Also changed - newA.dataArray[i] = buff[0]; Sorry for all the mistakes, they happened during code abstraction. — Nitkov, Sep 05 '14 at 13:16
@everyone -> I tried valgrind and it pointed me to a index out of range write that I kept overlooking. The problematic part was not visible in my abstracted question. That being said, all of your answers are probably right and I should modify my code according to them, however I am baffled as to which one to choose as correct when the actual solution was in the comments by Joachim. — Nitkov, Sep 08 '14 at 08:16

score 1 · Accepted Answer · answered Sep 05 '14 at 12:53

1

You have memory leaks there. You have to free the previously allocated memory in newA.dataArray, before you assign there a new memory.

As stated by Joachim, read operation is very time consuming and you shall read data in blocks to minimize overhead.

Additionally, you have to close file descriptors, otherwise they will be depleted soon.

answered Sep 05 '14 at 12:53

vookimedlo

1,209
12
24

I should free newA.dataArray before doing newA.dataArray = copyOfDataArray? – Nitkov Sep 05 '14 at 13:18
1

Exactly. In C, you have be careful since you are responsible for the memory management. – vookimedlo Sep 05 '14 at 13:21

pradipta · Answer 2 · 2014-09-05T13:31:46.123

0

There are many issue on the code as already given by others. Please checks bellow

typedef struct A{
int size;
char * dataArray;
}A

A load(char* filename, int inSize)
{
    A newA;
    newA.size = inSize;
    FILE *filePtr = NULL ; //Use NULL 
    char buff[1]; //Size of buffer is only 1 ,If needed increase that to copy more at a time
    int i = 0;

filePtr = fopen(filename,"rb");    
//Try to check for the filePtr == NULL or not

newA.dataArray = ( char*)malloc(sizeof(char) * newA.size);
//Same checking should be done here

for (i = 0; i < size; i++) //What is size 
{
    fread(buff, sizeof(char), 1, filePtr);
    newA.dataArray[i] = char[0]; //What is char[0]
}
//instead this you can read the bytes in a single call, use that.
// fread(buff, sizeof(char), <size to read >, filePtr);

char* copyOfDataArray = (char*)malloc(sizeof(char) * newA.size);

for (i = 0; i < size; i++)
{
    fread(buff, sizeof(char), 1, filePtr);
    copyOfDataArray[i] = newA.dataArray[i];
}

//why reading again once you done above.
newA.dataArray = copyOfDataArray;

return newA; //Please check: How you can return a auto variable.
}
void Initialize()
{
    A first = load("file1", 100);
    A second = load("file2", 20);
}

edited Sep 05 '14 at 13:31

answered Sep 05 '14 at 13:07

pradipta

1,718
2
13
24

newA.dataArray = copyOfDataArray; -> In the real code , copyOfDataArray is changed a bit ; return newA; //Please check: How you can return a auto variable. -> This may be the problem, what is a better way to do this? ; //Try to check for the filePtr == NULL or not -> I removed checking so that the question is not too long ; newA.dataArray[i] = char[0]; //What is char[0] -> i edited that, should be buffer[0] ; //instead this you can read the bytes in a single call, use that. // fread(buff, sizeof(char), , filePtr); -> will try – Nitkov Sep 05 '14 at 13:21
If you are not required to a thread safe code then you can go for a global variable, Just declare that variable as global.If needed to thread safe then you can use locks. – pradipta Sep 05 '14 at 13:26
newA.dataArray[i] = char[0]; //What is char[0] -> i edited that, should be buffer[0] : Here Please check the size of buffer should be long enough to hold the data.In your code it is 1 only. – pradipta Sep 05 '14 at 13:31
Please select as answer if it helps..we want only that. – pradipta Sep 05 '14 at 18:52
Sorry, I was out for the weekend, will try today and select if it works. – Nitkov Sep 08 '14 at 06:16

Dynamic array inside struct and malloc fail

2 Answers2