Contiguous malloc'd solution causing valgrind errors in tmc test

[mjb]

Hi,

I'm having problems with Module 4 exercise 2  on course Aalto C - 2016.

Relevant tmc pastes:

contiguous malloc version: https://tmc.mooc.fi/mooc/paste/3gps8sfwA1VnZva0O92Xkw

noncontiguous malloc version: https://tmc.mooc.fi/mooc/paste/H2G5E6gxRgVjb_-AeY7Djw

When using malloc to allocate the whole 2d array contiguously and managing pointers, I get valgrind errors when running tmc test and tmc submit, whereas noncontiguously I don't.

Curiously, the provided src/main.c doesn't have any memory leaks, with both versions, and I cannot see anything in test_source.c that would lead to one.

Any idea why this is happening? As far as I can see the mallocs and frees are done correctly. I've finished the question, but would prefer to know why this (supposedly superior) method of allocating the array would fail the test.

Best regards, George

[gebu...@googlemail.com]

bumping this. while it doesn't matter for completing the course it seems important to figure out.

for easier reference, the only differences between the two sets of codes:

in createField:

for(unsigned int i=0;i<ysize;i++)

    {

        playfield->cells[i] = malloc(xsize*sizeof(State));

    }

versus

playfield->cells[0] = malloc(xsize*ysize*sizeof(State));

     

    for(unsigned int i=0;i<ysize;i++)

    {

        playfield->cells[i] = playfield->cells[0] + i*xsize;

    }

 

and in releaseField:

for(unsigned int i=0;i<f->ysize;i++)

    {

        free(f->cells[i]);

    }

versus

free(f->cells[0]);

both should be valid ways to allocate memory, and it works for running main, but not for tmc test 

[ljleppan]

Hi,

My C is a bit rusty, but here's my understanding of what's going on: Valgrind is essentailly saying that you are not freeing all the memory you (or in this case, tests) allocate. It seems that in the failing version of the code you only free the first inner array of the array of arrays that is called "cells", while in the passing version you free them all. So likely valgrind notices that not all of the inner arrays are free'd and throws a fit over that.

-L

[gebu...@googlemail.com]

Hello :)

When counting the mallocs and frees, and looking at the memory, for the code provided in main, there is absolutely 0 memory leaks with both methods. The call to free should, and appears to for running the provided main, release all the memory of the 2d array's cells in one call.

I'm still unsure why when running the test it would fail and when running main it works perfectly. :S

[ljleppan]

Hi,

This is going to be long, sorry for that. Also, I'll be going over this in pretty close detail since this is actually a pretty interesting case and can maybe be of help to others in the future as well.  So if you are familiar with most of the stuff, you can probably skip to the end :)

OK, so let's take a look at the valgrind report in detail. So we have our valgrind report, the first error of which is 

==389== 448 bytes in 16 blocks are definitely lost in loss record 77 of 79
==389==    at 0x4C28C20: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==389==    by 0x401E2A: fieldFromString (test_source.c:107)
==389==    by 0x401FEA: test_printField (test_source.c:176)
==389==    by 0x406787: srunner_run (in /tmc/test/test)
==389==    by 0x4025B1: tmc_run_tests (tmc-check.c:122)
==389==    by 0x402271: main (test_source.c:225)
==389== 

Reading from top to bottom, we first get a description of the problem. Some allocated blocks are getting lost, meaning that we are allocating some memory that is not getting free'd during the lifetime of the program run, or in this case the test run. The next lines are a stack trace that details where the memory was allocated. So it was allocated using a malloc that happens on line 107 of test_source.c in the function fieldFromString, which was called from line 176 of the same file, from the function test_printField etc. But to actually understand what is going on, we need to figure out where that memory ends up.

test_source.c is the file that defines (some) of the tests used to assess your program and it can be found in the /test folder of the project. So let's look at line 107 of that, together with some surrounding lines (some comments added and parts omitted).

Field *fieldFromString(const char *s)
{
    // <omitted>

    // set up field
    Field *f = malloc(sizeof(Field));
    if (!f)
        return NULL;


    f->cells = malloc(sizeof(State*) * ysize);
    if (!f->cells) {
        free(f);
        return NULL;
    }
    for (unsigned int i = 0; i < ysize; i++) {
        f->cells[i] = malloc(sizeof(State) * xsize); // <- line 107
    }

    // <omitted>
    return f;
}

We see that as part of the tests, the tests are allocating a Field using a non-contingous malloc. 

Looking at the surroundings of line 176 next, we see the following (comments added):

START_TEST(test_printField)
{
    Field *f;
    for (unsigned int i = 0; i < sizeof(fields) / sizeof(char *); i++) {
        f = fieldFromString(fields[i]); // <- line 176, generates a known Field
       
        char str[400];
        fieldToString(f, str, sizeof(str)); // <- this calls your code, transforming it to a string
   
        char infostr[400];
        if (mycompare(str, fields[i], infostr) != 0) { // <- checks that the string is as expected
            releaseField(f); # <- line 183, this calls your code, releasing the Field
            fail("[M4.02.c] Your output:\n%s\nReference output:\n%s\nReason: %s\n",
                    str, fields[i], infostr);
        }
    releaseField(f); // <- line 187, this also calls your code, releasing the field
    }
}
END_TEST

This is one of the test cases. So the tests allocate a Field using the method in the tests. This Field is then compared to what your code generates given the same input. So essentially we are checking that your function fieldToString produces the expected output given a known Field. So far all good. 

But then we notice lines 183 and 187. The tests use *your code* to free the Field it created using multiple calls to malloc! And **that** is the problem. Your code assumes that the Field it free's was malloc'd in a certain way, which is not the same way the test mallocs its Field. 

**So in essence**: your code works correctly, **given that the Field is created using a contiguous malloc**, but the tests call your code using a Field created using a non-contiguous malloc. Just running your code in isolation, this issue will naturally never crop up. The issue is only in the interaction between the test cases and your code :)

Hopefully this clears the issue :) I'll email the person behind the tests to see if we could either make the test files also free their own memory (Which has some issues vis-a-vis the test file having most of the solution) or adding a note to the exercise prompt.

-L

[mjb]

Thanks so much for figuring this out.

I had tried reading through the test files, but I had apparently missed that important point.

Thanks for taking the time to sleuth it out :)