I am trying to write a set of functions that will support a dynamically allocated array where a struct contains the array and other metadata. The goal is to return the function to the user, and the struct information can be called from a function. The code seems to work just fine until I get to the function to free the memory from heap. For reasons I do not understand, the code fails with a segmentation fault, which would indicate that the variable vec in the free_vector function is not pointing to the correct address. However, I have verified with print statements that it is pointing to the correct address. I am hoping someone can help me understand why the free_vector function is not working, specifically the free command. My code and implementation is shown below.

typedef struct
{
    size_t allocated_length;
    size_t active_length;
    size_t num_bytes;
    char *vector;
} Vector;

void *init_vector(size_t num_indices, size_t num_bytes) {
    // Allocate memory for Vector struct
    Vector *vec = malloc(sizeof(*vec));
    vec->active_length = 0;
    vec->num_bytes = num_bytes;

    // Allocate heap memory for vector
    void *ptr = malloc(num_bytes * num_indices);
    if (ptr == NULL) {
        printf("WARNING: Unable to allocate memory, exiting!\n");
        return &vec->vector;
    }
    vec->allocated_length = num_indices;
    vec->vector = ptr;
    return &vec->vector;
}
// --------------------------------------------------------------------------------

int push_vector(void *vec, void *elements, size_t num_indices) {
    Vector *a = get_vector_data(vec);
    if(a->active_length   num_indices > a->allocated_length) {
        printf("TRUE\n");
        size_t size = (a->allocated_length   num_indices) * 2;
        void *ptr = realloc(a->vector, size * a->num_bytes);
        if (ptr == NULL) {
            printf("WARNING: Unable to allocate memory, exiting!\n");
            return 0;
        }
        a->vector = ptr;
        a->allocated_length = size;
    }
    memcpy((char *)vec   a->active_length * a->num_bytes, elements,
            num_indices * a->num_bytes);
    a->active_length  = num_indices;
    return 1;
}
// --------------------------------------------------------------------------------

Vector *get_vector_data(void *vec) {
    // - The Vector struct has three size_t variables that proceed the vector
    //   variable.  These variables consume 24 bytes of daya.  THe code below
    //   points backwards in memory by 24 bytes to the beginning of the Struct.
    char *a = (char *)vec - 24;
    return (Vector *)a;
}
// --------------------------------------------------------------------------------

void free_vector(void *vec) {
    // Free all Vector struct elements
    Vector *a = get_vector_data(vec);
    // - This print statement shows that the variable is pointing to the
    //   correct data.
    printf("%d\n" ((int *)vec)[2]);
    // The function fails on the next line and I do not know why
    free(a->vector);
    a->vector = NULL;
    a->allocated_length = 0;
    a->active_length = 0;
    a->num_bytes = 0;
}

int main() {
    int *a = init_vector(3, sizeof(int));
    int b[3] = {1, 2, 3};
    push_vector(a, b, 3);
    // The code begins to fails here
    free_vector(a);
}

CodePudding user response：

This program suffers from Undefined Behaviour.

The return value from init_vector is of type char **, a pointer-to-pointer-to-char,

return &vec->vector;

converted to void *.

In main, this value is converted to an int *

int *a = init_vector(3, sizeof(int));

This value is then converted back into a void * when passed to push_vector.

In push_vector, this value is cast to a char * in order to perform pointer arithmetic

memcpy((char *)vec   a->active_length * a->num_bytes, elements,
            num_indices * a->num_bytes);

where this operation overwrites the original pointer returned by malloc contained in the vector member.

On my system, this attempts to write 12 bytes (three int) to memory starting with the position of the vector member in the Vector structure.

Vector *vec
|                    &vec->vector
|                    |      
v                    v      
 ------ ------ ------ ------ ----- 
|size_t|size_t|size_t|char *|?????|
 ------ ------ ------ ------ ----- 

This overflows, as sizeof (char *) is 8 on my system.

This is the wrong place to write data. The correct place to write data is *(char **) vec - or just a->vector.

If the write does not crash the program directly (UB), this surely results in free being passed a pointer value that was not returned by malloc, calloc, or realloc, or the pointer value NULL.

Aside: In free_vector, this value is also cast to an int *

printf("%d\n", ((int *)vec)[2]); /* added a missing semi-colon. */

Additionally, it is unclear if free_vector should free the original allocation, or just the vector member. You do go to lengths to zero-out the structure here.

Still, as is, you have a memory leak - albeit a small one.

void free_vector(void *vec) {
    Vector *a = get_vector_data(vec);
    /* ... */
    free(a); /* This has to happen at some point. */
}

Note, you should be using offsetof to calculate the position of members within a structure. A static offset of 24 assumes two thing that may not hold true:

• sizeof (size_t) is always 8 (actual minimum sizeof (size_t) is 2), and
• the structure contains no padding to satisfy alignment (this seems likely given the form, but not strictly true).

The source you linked in the comments uses a flexible array member, not a pointer member, meaning the entirety of the data (allocation sizes and the vector) is stored in contiguous memory. That is why the & operator yields a valid location to copy data to in this implementation.

(Aside: the linked implementation appears to be broken by effectively using sizeof to get the base of the container structure from a pointer to the flexible array member (e.g., &((vector_container *) pointer_to_flexible_member)[-1]), which does not take into account the possibility of trailing padding, which would result in a larger offset than expected.)