Incorrect pointer created within a function

Question

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

vector_copy looks pretty broken. v2 is undefined, but you immediately try to do a memcpy into it. You need to allocate memory for it first, e.g. via malloc. — Tom Karzes
– Tom Karzes, Commented Mar 22, 2020 at 7:15
vector* newv; newv->items you dereference an uninitialized pointer — M.M
– M.M, Commented Mar 22, 2020 at 7:32

paxdiablo · Accepted Answer · 2020-03-22 12:24:42Z

1

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

edited Mar 22, 2020 at 12:24

answered Mar 22, 2020 at 7:16

paxdiablo

888k243 gold badges1.6k silver badges2k bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

paxdiablo Over a year ago

@Eric, there's a subtle distinction between "pointing to probably something" and "probably pointing to something". The former is a definite statement, the latter (what I meant) a likely situation based on the way most implementations work. If you call a function then return, then call another function, the stack frame will often be in the same memory area so uninitialised locals will probably get values from the first call. It's not guaranteed (hence my "probably" comment) but it is likely. You're correct in that it's no way mandated so I'll try make that clearer ...

paxdiablo Over a year ago

@Eric: Ah, never mind, I think I see what you're getting at. My original language, now changed, could be easily read as v2 pointing to some point on the stack. I've clarified it to now state that the v2 pointer itself (not what it points to) is the probable stack-leftover info. Hopefully that's clearer. If not, I'm open to any suggestions you may have on improving.

Collectives™ on Stack Overflow

Incorrect pointer created within a function

1 Answer 1

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related