Search code examples
cscopedynamic-memory-allocationstorage-duration

Segementation fault whilst trying to deep copy struct


I have this piece of code, where I have an array of Constituencies and I wish to deep copy the whole array into a new array. Now too me, it seems I'm doing this correctly but I keep getting a segmentation fault for this code. Is anybody able to spot what I'm doing wrong?

From what I can tell is going wrong is that when freeing the heap memory in the old array, this is somehow freeing the new heap memory stored in the new array. But I'm really not sure how this is the case?

main.c

int main() {
    const int numConstituencies = 5;
    struct Constituency constituencies[numConstituencies];

    constructConstituency(
        &constituencies[0], "London",
        (unsigned int[]){1, 2, 3, 4}, 4);
    constructConstituency(
        &constituencies[1], "Manchester",
        (unsigned int[]){0, 2}, 2);
    constructConstituency(
        &constituencies[2], "Leeds",
        (unsigned int[]){0, 1, 3}, 3);
    constructConstituency(
        &constituencies[3], "Liverpool",
        (unsigned int[]){0, 2, 4}, 3);
    constructConstituency(
        &constituencies[4], "Newcastle",
        (unsigned int[]){0, 3}, 2);

    struct Constituency * copy = copyConstituencies(constituencies,
                                                    numConstituencies);

    int i;
    for (i = 0; i < numConstituencies; i++) {
        destructConstituency(&constituencies[i]);
    }

    for (i = 0; i < numConstituencies; i++) {
        destructConstituency(&copy[i]);
    }
    return 0;

constituency.c

struct Constituency {
    char *name;
    unsigned int *neighbours;
    unsigned int numNeighbours;
};

void clearValues(struct Constituency * const constituency) {
    constituency->name = NULL;
    constituency->neighbours = NULL;
    constituency->numNeighbours = 0;
}

void constructConstituency(struct Constituency * const constituency,
                           char * const name,
                           unsigned int * const neighbours,
                           unsigned int numNeighbours) {

    clearValues(constituency);

    int nameLength = strlen(name) + 1; // \0 terminated string

    constituency->numNeighbours = numNeighbours;
    constituency->name = (char *) malloc(sizeof(char) * nameLength);
    constituency->neighbours = (unsigned int *) malloc(sizeof(unsigned int) * numNeighbours);
   if (constituency->name == NULL || constituency->neighbours == NULL) {
       printf("Unable to allocate for constituency: %s\n", name);
       exit(1);
   }

    memcpy(constituency->name, name, sizeof(char) * nameLength);
    memcpy(constituency->neighbours, neighbours, sizeof(unsigned int) * numNeighbours);
}

struct Constituency * copyConstituencies(struct Constituency * const constituencies,
                                         unsigned int const numConstituencies) {

    struct Constituency newConstituencies[numConstituencies];

    int i;
    for (i = 0; i < numConstituencies; i++) {
        constructConstituency(&newConstituencies[i],
                              constituencies[i].name,
                              constituencies[i].neighbours,
                              constituencies[i].numNeighbours);
    }

    return newConstituencies;
}

void destructConstituency(struct Constituency * const constituency) {
    free(constituency->name);
    free(constituency->neighbours);

    clearValues(constituency);
}

Solution

  • In this functipn

    struct Constituency * copyConstituencies(struct Constituency * const constituencies,
                                             unsigned int const numConstituencies) {
    
        struct Constituency newConstituencies[numConstituencies];
    
        int i;
        for (i = 0; i < numConstituencies; i++) {
            constructConstituency(&newConstituencies[i],
                                  constituencies[i].name,
                                  constituencies[i].neighbours,
                                  constituencies[i].numNeighbours);
        }
    
        return newConstituencies;
    }
    

    you are returning a pointer to a local array that will not be alive after exiting the function. So the returned pointer is invalid and dereferencing the pointer invokes undefined behavior.

    You need to allocate the array dynamically:

    
        struct Constituency *newConstituencies = malloc(sizeof(struct Constituency) * numConstituencies);