Search code examples
cmallocdynamic-memory-allocationc99opaque-types

Why is this allocation not well done?

I have point.h and polygon.h files, with their associated .c files. In point.h

// point.h
#ifndef POINT_H
#define POINT_H

typedef struct Point point;

point *point_alloc(void);
void *point_free(point *);
void *point_init(point *, float, float);
void *point_print(const point *);
float point_get_x(const point *);
float point_get_y(const point *);
void *point_set_x(point *, float);
void *point_set_y(point *, float);

#endif

In point.c, I have

// point.c
#include <stdlib.h>
#include <stdio.h>
#include "point.h"

#define GET_VALUE_ERROR (2L)

struct Point
{
    float x;
    float y;
};

point *point_alloc(void)
{
    point *pt = malloc(sizeof *pt);

    if(NULL == pt)
    {
        fprintf(stderr, "Could not allocate point.\n");
        return NULL;
    }
    else
    {
        return pt;
    }
}

void *point_free(point *pt)
{
    if(NULL == pt)
    {
        fprintf(stderr, "Could not free point.\n");
        return NULL;
    }
    else
    {
        free(pt);
        pt = NULL;
        return NULL;
    }
}

void *point_init(point *pt, float x, float y)
{
    if(NULL == pt)
    {
        fprintf(stderr, "Cannot initiate point.\n");
        return NULL;
    }
    else
    {
        pt -> x = x;
        pt -> y = y;
        return NULL;
    }
}

void *point_print(const point *pt)
{
    if(NULL == pt)
    {
        fprintf(stderr, "Cannot print point.\n");
        return NULL;
    }
    else
    {
        printf("Point at (%f, %f)\n", pt -> x, pt -> y);
        return NULL;
    }
}

float point_get_x(const point *pt)
{
    if(NULL == pt)
    {
        fprintf(stderr, "Cannot get point.\n");
        return GET_VALUE_ERROR;
    }
    else
    {
        return pt -> x;
    }
}

float point_get_y(const point *pt)
{
    if(NULL == pt)
    {
        fprintf(stderr, "Cannot get point.\n");
        return GET_VALUE_ERROR;
    }
    else
    {
        return pt -> y;
    }
}

void *point_set_x(point *pt, float x)
{
    if(NULL == pt)
    {
        fprintf(stderr, "Cannot get point.\n");
        return NULL;
    }
    else
    {
        pt -> x = x;
        return NULL;
    }
}

void *point_set_y(point *pt, float y)
{
    if(NULL == pt)
    {
        fprintf(stderr, "Cannot get point.\n");
        return NULL;
    }
    else
    {
        pt -> y = y;
        return NULL;
    }
}

while in polygon.h I have

// polygon.h
#ifndef POLYGON_H
#define POLYGON_H

typedef struct Polygon polygon;

polygon *polygon_alloc(unsigned);
void *polygon_free(polygon *);
void *polygon_init(polygon *, unsigned, float, point *);
void *polygon_print(const polygon *);
unsigned polygon_get_nside(const polygon *);
void *polygon_set_nside(polygon *, unsigned);
point *polygon_get_centre(const polygon *);
void *polygon_set_centre(polygon *, point *);
point *polygon_get_vertex(const polygon *, unsigned);
void *polygon_set_vertex(polygon *, unsigned, float, float);

#endif

and in polygon.c I have

// polygon.c
#include <math.h>
#include <stdio.h>
#include <stdlib.h>
#include "point.h"
#include "polygon.h"

#ifndef M_PI
    #define M_PI (3.14159265358979323846264338327950288)
#endif

#define GET_NSIDE_ERROR (2U)

struct Polygon
{
    unsigned nside;
    point *centre;
    point **vertices;
};

polygon *polygon_alloc(unsigned nside)
{
    if(nside == 0)
    {
        fprintf(stderr, "Cannot have a 0-sided polygon.\n");
        return NULL;
    }
    else
    {
        polygon *poly = malloc(sizeof(*poly));

        if(NULL == poly)
        {
            fprintf(stderr, "Cannot allocate polygon.\n");
            return NULL;
        }
        else
        {
            poly -> nside = nside;
            poly -> centre = point_alloc();
            if(NULL == poly -> centre)
            {
                fprintf(stderr, "Cannot allocate polygon centre.\n");
                free(poly);
                poly = NULL;
                return NULL;
            }
            else
            {
                poly -> vertices = malloc(nside * sizeof(point*));
                if(NULL == poly -> vertices)
                {
                    fprintf(stderr, "Cannot allocate polygon vertices.\n");
                    free(poly -> centre);
                    poly -> centre = NULL;
                    free(poly);
                    poly = NULL;
                    return NULL;
                }
                else
                {
                    for(unsigned i = 0; i < nside; i++)
                    {
                        poly -> vertices[i] = point_alloc();
                        if(NULL == poly -> vertices[i])
                        {
                            fprintf(stderr, "Cannot allocate node %u.\n", i);
                            for(unsigned j = 0; j < i; j++)
                            {
                                free(poly -> vertices[j]);
                                poly -> vertices[j] = NULL;
                            }
                            free(poly -> centre);
                            poly -> centre = NULL;
                            free(poly -> vertices);
                            poly -> vertices = NULL;
                            free(poly);
                            poly = NULL;
                        }
                    }
                }
            }
        }

        return poly;
    }
}

void *polygon_free(polygon *poly)
{
    if(NULL == poly)
    {
        fprintf(stderr, "Cannot free polygon.\n");
        return NULL;
    }
    else
    {
        if(NULL == poly -> centre)
        {
            fprintf(stderr, "Cannot free polygon centre.\n");
            return NULL;
        }
        else
        {
            free(poly -> centre);
            poly -> centre = NULL;

            if(NULL == poly -> vertices)
            {
                fprintf(stderr, "Cannot free polygon vertices.\n");
                return NULL;
            }
            else
            {
                for(unsigned i = 0; i < poly -> nside; i++)
                {
                    if(NULL == poly -> vertices[i])
                    {
                        fprintf(stderr, "Cannot free vertex %u.\n", i);
                        return NULL;
                    }
                    else
                    {
                        free(poly -> vertices[i]);
                        poly -> vertices[i] = NULL;
                    }
                }

                free(poly -> vertices);
                poly -> vertices = NULL;
            }
        }

        free(poly);
        poly = NULL;
    }

    return NULL;
}

void *polygon_init(polygon *poly, unsigned nside, float radius, point *centre)
{
    if(NULL == poly)
    {
        fprintf(stderr, "Polygon not existing.\n");
        return NULL;
    }
    else if(NULL == centre)
    {
        fprintf(stderr, "Polygon centre not existing.\n");
        return NULL;
    }
    else
    {
        polygon_set_nside(poly, nside);
        polygon_set_centre(poly, centre);

        for(unsigned i = 0; i < poly -> nside; i++)
        {
            float angle = (1 + 2 * i) * (M_PI / poly -> nside);
            float x = radius * cos(angle);
            float y = radius * sin(angle);
            polygon_set_vertex(poly, i, x, y);
        }

        return NULL;
    }
}

void *polygon_set_nside(polygon *poly, unsigned nside)
{
    if(NULL == poly)
    {
        fprintf(stderr, "Cannot set polygon number of sides.\n");
        return NULL;
    }
    else
    {
        poly -> nside = nside;
        return NULL;
    }
}

void *polygon_set_centre(polygon *poly, point *centre)
{
    if(NULL == poly)
    {
        fprintf(stderr, "Cannot set polygon number of sides.\n");
        return NULL;
    }
    else
    {
        poly -> centre = centre;
        return NULL;
    }
}

// Rest of implementation

Apparently, the allocation and/or free of the polygon is not correct. To see this, I wrote this main.c reading

#include <stdio.h>
#include <stdlib.h>
#include "point.h"
#include "polygon.h"

int main() {
    unsigned nside = 3;
    float radius = 1.0;

    point* centre = point_alloc();
    point_init(centre, 0.0, 0.0);

    polygon* poly = polygon_alloc(nside);
    polygon_init(poly, nside, radius, centre); // this one is a problem
    polygon_print(poly);

    polygon_free(poly);
    point_free(centre);
    exit(EXIT_SUCCESS);
}

compiled using the command gcc -Wall -Wextra -Wpedantic -Werror -std=c99 .\point.c .\polygon.c .\main.c -o .\main.exe, I have a a "red cross in my IDE (I use VSCode), as illustrated here in the picture (which indicates that something is wrong) and I do not know neither from where it comes, nor how to solve it? enter image description here

EDIT

I corrected the allocation/freeing function thanks to the answer below, and was able to find that the problem is caused by the polygon_init function, but I cannot pinpoint what is wrong there?

Solution

  • Your program segfaults in free_polygon() because you free the vertices array before each of its verticies[i] element:

                    free(poly -> vertices);
                poly -> vertices = NULL;

                for(unsigned i = 0; i < poly -> nside; i++)
                {
                    if(NULL == poly -> vertices[i])
                    {
                        fprintf(stderr, "Cannot free vertex %u.\n", i);
                        return NULL;
                    }
                    else
                    {
                        free(poly -> vertices[i]);
                        poly -> vertices = NULL;
                    }
                }

    In general you want to free resources in reverse order of how they are allocated (free vertices[i] before vertices). Any error in free_polygon() may result in memory leaks. For example if poly->vertices[i] == NULL then we don't free remaining poly->verticies[i], poly->vertices, poly->centre or poly. As your alloc_polygon() takes care of partial constructions is probably ok but it's a risky design:

    void* free_polygon(polygon* poly) {
    if(NULL == poly) {
        fprintf(stderr, "Cannot free polygon.\n");
        return NULL;
    }
    if(NULL == poly->vertices) {
        fprintf(stderr, "Cannot free polygon vertices.\n");
        return NULL;
    }
    for(unsigned i = 0; i < poly->nside; i++) {
        if(NULL == poly->vertices[i]) {
            fprintf(stderr, "Cannot free vertex %u.\n", i);
            return NULL;
        }
        free(poly->vertices[i]);
        poly->vertices[i] = NULL;
    }
    free(poly->vertices);
    if(NULL == poly->centre) {
        fprintf(stderr, "Cannot free polygon centre.\n");
        return NULL;
    }
    free(poly->centre);
    free(poly);
    return NULL;
}

    Other suggestions:

    1. As free_polygon() always return NULL consider changing it's return type to void.
    2. With experience !poly is easier to read then NULL == poly.
    3. alloc_polygon() is quite complicated; consider designing the allocation and free function as a matching pair so you can always call free_polygon() on a partially constructed object by initializing variables with NULL, calloc instead of malloc() of verticies and free resources in reverse. I routinely use goto for cleanup chains but if you have qualms you can duplicate those two lines instead in each error case. Compilers (at least gcc) will warn you if you miss a variable initialization before a goto which I find helpful.
    4. As your poly object's lifetime expired after a call to free_polygon() I never bother setting any of the variables to NULL. That said it may help you catch a use after free defect.
    5. Consider using a namespace prefix, i.e. polygon_alloc() instead of alloc_polygon().
    6. Prefer polygon *poly to polygon* poly. It avoids confusion if you ever declare two pointers on one line.
    polygon *polygon_alloc(unsigned nside) {
    polygon *poly = malloc(sizeof *poly);
    if(!poly) {
        fprintf(stderr, "Cannot allocate polygon.\n");
        goto err;
    }
    poly->nside = nside;
    poly->vertices = NULL;
    poly->centre = alloc_point();
    if(!poly->centre) {
        fprintf(stderr, "Cannot allocate polygon centre.\n");
        goto err;
    }
    poly->vertices = calloc(nside, sizeof *poly->vertices);
    if(!poly->vertices) {
        fprintf(stderr, "Cannot allocate polygon vertices.\n");
        goto err;
    }
    for(unsigned i = 0; i < nside; i++) {
        poly->vertices[i] = alloc_point();
        if(!poly->vertices[i]) {
            fprintf(stderr, "Cannot allocate node %u.\n", i);
            goto err;
        }
    }
    return poly;
:err
    polygon_free(poly);
    return NULL;   
}

void polygon_free(polygon *poly) {
    if(!poly) return;
    if(poly->vertices)
        for(unsigned i = 0; i < poly->nside; i++)
           free(poly->vertices[i]);
    free(poly->vertices);
    free(poly->centre);
    free(poly);
}