CODEWITHSUNDEEP
csegmentation-fault
skeytrek
skeytrek

Reputation: 53

Incorrect pointer created within a function

I'm trying to implement an integer vector in C. The vector pointer created in vector_copy is aroung 0x3 or 0x2, which causes segmentation fault. Why is that happenning? This really baffles me since I have a similar implementation for vector_new.

typedef struct vector {
    int* items;
    size_t size;
    size_t max;
} vector;

vector* vector_copy(vector* v) {
    vector* v2;

    memcpy(v2->items,v->items,v->max * sizeof(int*));
    if (v2->items == NULL) {
        return NULL;
    }
    v2->size = v->size;
    v2->max = v->max;
    return v2;
}

vector* vector_new(size_t initial_capacity) {
    vector* newv;
    newv->items = malloc(initial_capacity * sizeof(int*));
    if (newv->items == NULL) {
        return NULL;
    }
    newv->size = 0;
    newv->max = initial_capacity;
    return newv;
}

int main() {
    vector* v;
    vector* v2;

    v = vector_new(4);
    //new vector with capacity 4 and size 0

    vector_push(v, 1);
    vector_push(v, 2);
    //1 and 2 are pushed back

    v2=vector_copy(v);
    //vector is supposed to be copied

    vector_free(v);
    vector_free(v2);
    return 0;
}

Upvotes: 0

Views: 59

Answers (1)

paxdiablo
paxdiablo

Reputation: 881363

vector* v2;
memcpy(v2->items,v->items,v->max * sizeof(int*));

This is not a good idea. You have basically created a pointer v2 that points to some arbitrary location (because you haven't initialised it), with the v2 pointer value probably being whatever was left over on the stack from some previous operation(a). Then, by copying bytes to that arbitrary location, all sorts of weirdness may ensue.

You need to actually allocate some memory for this new vector, with something like:

vector *v2 = vector_new(v->max);
memcpy(v2->items, v->items, v->max * sizeof(int));

You'll (hopefully) also notice I've changed the sizeof to use int rather than int*. I suspect that items points to an array of the former in which case that's the correct value to use.

So the following function is a better starting point:

vector *vector_copy(vector* v) {
    // Use vector_new to get empty one with exact same properties.

    vector *v2 = vector_new(v->max);
    if (v2 == NULL) return NULL;

    // Copy data in to it and return.

    memcpy(v2->items, v->items, v->max * sizeof(int));
    v2->size = v->size;

    return v2;
}

You also have the exact same issues (no structure allocation and wrong sizeof) in your new function, which can be fixed with:

vector *vector_new(size_t initial_capacity) {
    // Allocate a vector structure, fail if no go.

    vector *newv = malloc(sizeof(vector));
    if (newv == NULL) return NULL;

    // Allocate the data area, free structure and fail if no go.

    newv->items = malloc(initial_capacity * sizeof(int));
    if (newv->items == NULL) {
        free(newv);
        return NULL;
    }

    // Set up everything needed and return it.

    newv->size = 0;
    newv->max = initial_capacity;

    return newv;
}

The fact that this function sets the maximum based on initial_capacity relieves you of the necessity of setting max in vector_copy().

(a) I state "probably" because of the way many implementations work in terms of reusing stack frame memory regions. However, it's by no means guaranteed, just one possibility. You should just assume that it will have some random value and will therefore behave appropriately (or inappropriately, depending on your viewpoint).

Upvotes: 1

Related Questions