How to fix unstable free() call on a ragged matrix inside a struct in C

Question

I'm creating a map with n * m dimensions, that should be gotten from a file input. Each position of the map matters for distance counting. In the whole execution cycle I go through 4 malloc() calls, however I can't match them with 4 free() calls or the execution might or might not finish and I can't understand why.

Here's the main call:

int main()
{
    map_t newMap;
    newMap = map_create(30, 30);

    //there's some test calls that I'm ommiting as they simply work and don't matter

    map_destroy(newMap);
    return 0;
}

the map_t as I understand and as I want is a pointer to a structure, it's defined on a header file as following:

typedef struct map *map_t;

it's implementation is on the corresponding .c file for matters of encapsulation. Here's the implementation and the important functions for the case:

struct map{
    position_t **positions;
    int lines, columns;
};

map_t map_create(int n, int m)
{
    map_t newMap = malloc(sizeof(map_t));

    newMap->lines = n;
    newMap->columns = m;

    newMap->positions = malloc(n * sizeof(position_t*));
    int i;
    int k;
    for(i = 0; i < n; ++i)
    {
        newMap->positions[i] = malloc(m * sizeof(position_t));
        for(k = 0; k < m; ++k)
        {
            newMap->positions[i][k] = position_create();
        }
    }

    return newMap;
}

void map_destroy(map_t map_p)
{
    int i, k;
    int n = map_p->lines;
    int m = map_p->columns;
    for(i=0; i < n; ++i)
    {
        for(k=0; k < m; ++k)
        {
            position_destroy(map_p->positions[i][k]);
        }
        free(map_p->positions[i]);
    }

    free(map_p->positions);

    free(map_p);
}

For matters of completeness, here's the position struct specific code, it follows the same idea of the map struct.

typedef struct position *position_t; //this is on the .h, the rest is on the .c file

struct position{
    int has_treasure;
    int times_visited;
};

position_t position_create()
{
    position_t newPosition = malloc(sizeof *newPosition);
    newPosition->has_treasure = YES;
    newPosition->times_visited = 0;

    return newPosition;
}

void position_destroy(position_t position_p)
{
    free(position_p);
}

I've identified which free calls are causing the problem, I just can't understand why. If I comment these 2 lines:

free(map_p->positions);
free(map_p);

everything works fine. I've seen different ways to allocate the initial memory such as this one - How do I correctly set up, access, and free a multidimensional array in C? - but even with the various solutions found there I keep getting the same unstability.

It might be extremely simple and I'm just overlooking something, been on this for 5 days now, as much I hate to borrow anyone's time on this mess, even if I have to implement a different solution I'd like to at least understand what's wrong.

Code isn't producing any warnings with -Wall -g -c and if I add -pedantic I just get complaints about comments.

Cheers in advance

EDIT: Changed position_t newPosition = malloc(sizeof(position_t)); to position_t newPosition = malloc(sizeof *newPosition); behaviour is still the same

EDIT2: did the exact type of change on the map_t malloc, problem solved, the whole time I thought the problem was on the **positions

Code like `position_t newPosition = malloc(sizeof(position_t));` will allocate the size of a pointer and not the strcuct you clearly are expecting .... — Soren, Apr 08 '16 at 01:22
I see no use of `position_t` being a pointer. Please don't be a [three-star programmer](http://c2.com/cgi/wiki?ThreeStarProgrammer). — Some programmer dude, Apr 08 '16 at 01:23
It say `typedef struct position *position_t;` so position_t is a pointer — Soren, Apr 08 '16 at 01:24
Regarding the comment from @Soren, this is why most experienced programmers avoid against creating type-aliases of pointers. When even you, who written the code and should know that e,g, `map_t` is a pointer, have trouble distinguishing the types and their pointers, it should make you think a little. Better use explicit pointer declarations when needed. — Some programmer dude, Apr 08 '16 at 01:25
@Soren but if that's the problem, wouldn't it give the problems related to the position struct instead of the map struct? — Kirag, Apr 08 '16 at 01:29
@Kirag -- you are writing in memory you have not allocated (your malloc is too small) so the entire program has undefined behavior -- who know what will happen, and how -- this may not be the exact problem you asked about which is why I didn't put it as an answer -- but fix you malloc size problems first — Soren, Apr 08 '16 at 01:32
The `position` structure has two `int` variables, each probably 32 bits, making the whole structure 64 bits in size. Which incidentally is the size of a pointer on a 64-bit system. — Some programmer dude, Apr 08 '16 at 01:33
@JoachimPileborg I did the typedef on purpose tho after this https://alastairs-place.net/blog/2013/06/03/encapsulation-in-c/ although I understand your point, and I know it is a pointer but has the _t suffix on the name, that might be a bad naming for others to read tho. In the end tho I can iterate and change the positions without problems, it's the last 2 free() calls that cause instability, can it be related? EDIT: just read your last reponse, does make sense then. Thank you!! — Kirag, Apr 08 '16 at 01:34
By the way, a quick tip on how to always get the right size for simple allocations: `position_t newPosition = malloc(sizeof *newPosition);` Note the dereferencing of `newPosition` in the call to `malloc`. — Some programmer dude, Apr 08 '16 at 01:35
@JoachimPileborg I should've tried that before, changed it but it keeps randomly crashing on the last 2 free calls, I'm starting to feel ashamed by this — Kirag, Apr 08 '16 at 01:49
@Kirag -- you also have the same problem with your map_t as it is also defined as a pointer — Soren, Apr 08 '16 at 01:49
@Soren totally right, it's working without any problems at all now. I really feel ashamed, but more importantly I feel grateful, thank you both, I can finally sleep — Kirag, Apr 08 '16 at 02:00

score 0 · Answer 1 · answered Apr 08 '16 at 02:07

0

The first malloc(sizeof(map_t)); allocates only space for a pointer. You need to use malloc(sizeof(*map_t));.

But you already found that now, I see.

answered Apr 08 '16 at 02:07

Aganju

6,295
1
12
23

How to fix unstable free() call on a ragged matrix inside a struct in C

1 Answers1