double free or corruption (out) in realloc

Question

I'm trying to fix a problem in my code since several days but I'm still stuck on it. I want to insert a value in a tab through a realloc but I have a memory leak (or something else) and I don't know why.

Here's my code:

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

typedef struct struct_vector{
    int nbElement;
    double* element;
}s_vector;

typedef s_vector* p_s_vector;

p_s_vector vector_alloc(size_t n){
    p_s_vector vect =(p_s_vector) malloc(sizeof(p_s_vector));
    vect->nbElement = n;
    vect->element = (double*) malloc(sizeof(double) * n);
    for(int i=0; i<n; i++){
        vect->element[i] = i; 
    }
    return vect;
}

void vector_free(p_s_vector p_vector){
    free(p_vector->element);
    free(p_vector);
    p_vector = NULL;
}

void vector_insert(p_s_vector p_vector, size_t i, double v){
    if(i < 0 || i > p_vector->nbElement)
        exit(0);
    else{   
        p_s_vector temp = vector_alloc(p_vector->nbElement);
        for(int k=0; k < temp->nbElement; k++)
            temp->element[k] = p_vector->element[k];
            
        p_vector->element = (double*)realloc(p_vector->element,sizeof(double)*(p_vector->nbElement++));
        for(int k=0; k<i; k++)
            p_vector->element[k] = temp->element[k];
            
        p_vector->element[i] = v;
        for(int k=i+1; k<p_vector->nbElement; k++)
            p_vector->element[k] = temp->element[k-1];
            
        vector_free(temp);
    }
}

int main(){
    p_s_vector vect = vector_alloc(3);
    
    vector_insert(vect, 1, 11);
    
    for(int i=0; i<vect->nbElement; i++){
        printf("%.1f\n",vect->element[i]);
    }
    
    vector_free(vect);
    return 0;
}

When I run my program, I have a double free or corruption (out) error. With Valgrind command, I have many "Address 0x4a4d048 is 0 bytes after a block of size 8 alloc'd" and I don't understand why

If someone can help me, it will be great. Thank you.

Answer 1

You have two errors in your code. The line

    p_s_vector vect = (p_s_vector) malloc(sizeof(p_s_vector));

should be

    p_s_vector vect = malloc(sizeof(s_vector));

since you want to allocate a structure and not a pointer to it (and don't cast the return value of malloc). Also you should pre-increment the count in the reallocation, so

    p_vector->element = (double *)realloc(p_vector->element, sizeof(double) * (p_vector->nbElement++));

should be

    p_vector->element = realloc(p_vector->element, sizeof(double) * (++p_vector->nbElement));

It's even better to stay away from side-effects in expressions so I would recommend incrementing the count in a separate statement.

To simplify memory allocations and make them less error prone I can recommend defining macro functions like this:

#include <errno.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define NEW_ARRAY(pointer, length) \
    { \
        (pointer) = malloc(((size_t) length) * sizeof (pointer)[0]); \
        if ((pointer) == NULL) { \
            fprintf(stderr, "Allocating %lu bytes of memory with malloc failed: %s\n", ((long unsigned int) length) * sizeof (pointer)[0], strerror(errno)); \
            exit(EXIT_FAILURE); \
        } \
    }

#define RENEW_ARRAY(pointer, length) \
    { \
        (pointer) = realloc((pointer), ((size_t) length) * sizeof (pointer)[0]); \
        if ((pointer) == NULL) { \
            fprintf(stderr, "Allocating %lu bytes of memory with realloc failed: %s\n", ((long unsigned int) length) * sizeof (pointer)[0], strerror(errno)); \
            exit(EXIT_FAILURE); \
        } \
    }

#define NEW(pointer) NEW_ARRAY((pointer), 1)

typedef struct struct_vector {
    int nbElement;
    double *element;
} s_vector;

typedef s_vector *p_s_vector;

p_s_vector vector_alloc(size_t n)
{
    p_s_vector vect;
    NEW(vect);
    vect->nbElement = n;
    NEW_ARRAY(vect->element, n);
    for (int i = 0; i < n; i++) {
        vect->element[i] = i;
    }
    return vect;
}

void vector_free(p_s_vector p_vector)
{
    free(p_vector->element);
    free(p_vector);
    p_vector = NULL;
}

void vector_insert(p_s_vector p_vector, size_t i, double v)
{
    if (i < 0 || i > p_vector->nbElement) {
        exit(0);
    } else {
        p_s_vector temp = vector_alloc(p_vector->nbElement);
        for (int k = 0; k < temp->nbElement; k++) {
            temp->element[k] = p_vector->element[k];
        }

        p_vector->nbElement++;
        RENEW_ARRAY(p_vector->element, p_vector->nbElement);

        for (int k = 0; k < i; k++) {
            p_vector->element[k] = temp->element[k];
        }

        p_vector->element[i] = v;
        for (int k = i + 1; k < p_vector->nbElement; k++) {
            p_vector->element[k] = temp->element[k - 1];
        }

        vector_free(temp);
    }
}

int main()
{
    p_s_vector vect = vector_alloc(3);

    vector_insert(vect, 1, 11);

    for (int i = 0; i < vect->nbElement; i++) {
        printf("%.1f\n", vect->element[i]);
    }

    vector_free(vect);
    return 0;
}

Finally, your code becomes less noisy if you drop the "p" and "s" prefixes; I would simply define the vector data type as

struct VectorDesc {
    int nbElement;
    double *element;
};

typedef struct VectorDesc *Vector;