Segementation fault whilst trying to deep copy struct

Question

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);
}

Please also read https://ericlippert.com/2014/03/05/how-to-debug-small-programs/. — Karl Knechtel, Feb 13 '22 at 15:16

Vlad from Moscow · Accepted Answer · 2022-02-13T15:16:45.730

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);

Alternately, you can use a pointer to an existing `Constituency[]`, *into which* the values will be copied - similar to how `constructConstituency` initializes already-allocated memory rather than `malloc`ing a `struct Constituency` and returning the pointer. — Karl Knechtel, Feb 13 '22 at 15:18

Segementation fault whilst trying to deep copy struct

1 Answers1