Keeping pointers to local scope variables

Question

First post, long time reader. I am learning C but coming from many years of experience with Python. Sorry if this is a repost, but I don't even know the correct vocabulary to use when searching Google.

I think I am still stuck in an object-oriented mindset, which leads me to to stupid things in C. I have a type "my_type" which contains a pointer to a second type "my_array". In my OO way of thinking, I want to write New and Free functions for my_type. During initialization, my_type should create a new my_array and store it's pointer. Later during destruction, the destructor for my_type should call the destructor for my_array and free the dynamically allocated memory.

This code compiles but yields a nasty error

#include <stdio.h>
#include <stdlib.h>


/**
* @brief A type implementing a dynamically allocated array
*/
typedef struct my_array {
    int n;
    double *array;
} my_array;


/**
* @brief A type containing a my_array pointer 
*/
typedef struct my_type {
    my_array *a;
} my_type;


/**
* @brief Initialize the dynamically allocated array
*
* @param ma pointer to a my_array
* @param n number of array elements
*/
void my_array_new(my_array *ma, int n) {
    ma->n = n;
    ma->array = (double *) malloc(n * sizeof(double));
}


/**
* @brief Free dynamically allocated memory of a my_array
*
* @param ma pointer to a my_array
*/
void my_array_free(my_array *ma) {
    free(ma->array);
}


/**
* @brief Initialize a my_type
*
* @param mt pointer to a my_type
* @param n number of array elements
*/
void my_type_new(my_type *mt, int n) {
    /* Create a local my_array */
    my_array a;
    my_array_new(&a, n);

    /* Store pointer to a */
    mt->a = &a;

    /* I am holding on to the pointer for a, but does a fall out of scope
     * here? Is this the origin of the error? */
}


/**
* @brief Free allocated my_type
*
* @param mt pointer to my_type
*/
void my_type_free(my_type *mt) {
    my_array_free(mt->a);
}


int main(int argc, char *argv[]) {
    my_type mt;
    int n = 10;

    printf("Initializing my_type %p\n", &mt);
    my_type_new(&mt, n);
    printf("mt.a->n = %d\n", mt.a->n);
    printf("Freeing my_type  %p\n", &mt);
    my_type_free(&mt);

    return 0;
}

Here is the error

Initializing my_type 0x7ffede7434c0
mt.a->n = 10
Freeing my_type  0x7ffede7434c0
*** Error in `./test': double free or corruption (out): 0x00007ffede7434c0 ***
======= Backtrace: =========
/lib64/libc.so.6[0x3e5f077d9e]
/lib64/libc.so.6(cfree+0x5b5)[0x3e5f0839f5]
./test[0x400618]
./test[0x400662]
./test[0x4006da]
/lib64/libc.so.6(__libc_start_main+0xf0)[0x3e5f01ffe0]
./test[0x4004f9]

I think I have an idea what is going on here. When I call my_type_new I create a local my_array variable. When the function ends this falls out of scope, but I still have the pointer stored. And now it gets fuzzy: is the memory formally assigned to mt.a marked as junk, but I can still read out it's value from dereferencing the pointer?

What is the correct way for my_type_new to create and store a my_array?

I am open to all suggestions, but I can't use C++ classes here for other reasons.

Answer 1

The issue here is that my_type is pointing to an object that was allocated on the stack and no longer exists. Thus, when you try to free it, you are getting an error because the free function knows that the block of memory you are trying to free isn't a block on the heap that can be deallocated.

The function that initializes a my_type instance should allocate space for a my_array if you want to do things this way. And the destructor should then free this space, in addition to the space for the array. Specifically:

void my_type_new(my_type *mt, int n) {
  /* Create a local my_array */
  mt->a = malloc(sizeof *mt->a);
  my_array_new(mt->a, n);
}

void my_type_free(my_type *mt) {
  my_array_free(mt->a);
  free(mt->a);
}

Ideally, you would also check for errors allocating enough space and NULL your pointers after freeing the memory blocks.

I might also mention that you may find it preferable to store these types of objects in place. In other words, if my_type will always need a single instance of my_array and you don't want it to ever be null, you could just include a my_array instance directly in the struct definition of my_type and then initialize it. This has the advantage of reducing the allocations and deallocations required by the objects you define. So, for example,

typedef struct my_type {
  my_array a;
my_type;

If you take this approach, then the initializer and destructor for my_type look something like this:

void my_type_new(my_type *mt, int n) {
  my_array_new(&mt->a, n);
}

void my_type_free(my_type *mt) {
  my_array_free(&mt->a);
}

Note that you no longer need to allocate extra space for my_array because it is included directly as a member of my_type.

As a note, this kind of object-oriented approach is perfectly valid. I prefer it, in part because it is so familiar to people who aren't terribly familiar with C. But also because I find it effective for organizing data and functionality.

You might be interested in this read.

Answer 2

The problem is in this function

void my_type_new(my_type *mt, int n) {
    /* Create a local my_array */
    my_array a;
    my_array_new(&a, n);

    /* Store pointer to a */
    mt->a = &a;

    /* I am holding on to the pointer for a, but does a fall out of scope
     * here? Is this the origin of the error? */
}

the my_array a; variable is local to the function, so it's automatically deallocated when the function returns and you can't use it after that because then the behavior is undefined, you are storing it's address in the a poitner of mt. You then free() it, which is yet another error.

It's still readable because it's not necessarily overwritten immediately after free(), it's just made available if more memory is requested from the OS.

You should malloc() it too, like this

void my_type_new(my_type *mt, int n) {
    /* Create a local my_array */
    my_array *a;
    a = malloc(sizeof(*a));
    if (a != NULL)
        my_array_new(a, n);
    /* Store pointer to a */
    mt->a = a;

    /* I am holding on to the pointer for a, but does a fall out of scope
     * here? Is this the origin of the error? */
}

since doing this could leave mt->a with NULL value, you must check against NULL before dereferencing it.

Answer 3

/* I am holding on to the pointer for a, but does a fall out of scope
 * here? Is this the origin of the error? */

Yes, a falls out of scope, causing you to lose reference to the memory you have malloced. This results in a memory leak; your program has consumed dynamic memory that it cannot free. Worse yet, because you've kept a reference to the variable that's fallen out of scope, your attempt to access that variable later causes undefined behaviour (the error you see).

Luckily for your case, the solution is simple enough. If you change void my_type_new(my_type *mt, int n) to my_array my_type_new(my_type *mt, int n) then you can return a;, and so long as you assign the return value of my_type_new into a my_array you can still access the memory in order to free it.

For example:

my_array my_type_new(my_type *mt, int n) {
    my_array a;
    my_array_new(&a, n);
    return a;
}

... an abridged example of using it:

my_array foo = my_type_new(...);

... and using your destructor:

my_type_free(&foo);

Answer 4

I write this as an answer, as it is too long for a comment and you asked for suggestions in general.

The approach you use will soon become a nightmare to maintain, not to talk about modify or refactor.

You approach using a chain of pointer is also a performance stopper, as all other objects have to be fetched explicitly.

If you use gcc and are allowed to use its extensions to the C language, have a look at the plan9-extensions. In addition with the standard (since C99) anonymous struct fields, this allows a much cleaner OOP approach.

Even with standard C99, you can get along much better with the latter feature and type casts.