qvector_t: calling addlast after calling clear causes double free and SIGABRT

[bryanwagner]

I think there may be an issue in the clear function in qvector_t (using C11) starting with the line:

free(vector->data);

I ran into this when using the library (which has been super valuable and useful, thank you!) and I tested it in isolation with this program (lines packed for brevity):

#include <stdio.h>
#include <qlibc/qlibc.h>

void succeeds() {
    printf("\nsucceeds:\n");
    qvector_t *vec = qvector(10, sizeof(uint64_t), QVECTOR_RESIZE_DOUBLE);
    uint64_t a = 1, b = 2, c = 3, d = 4, e = 5;
    printf("addlast: &a"); vec->addlast(vec, &a);
    printf(", &b"); vec->addlast(vec, &b);
    printf(", &c"); vec->addlast(vec, &c);
    printf(", &d"); vec->addlast(vec, &d);
    printf(", &e"); vec->addlast(vec, &e);
    printf(", size=%zu\n", vec->size(vec));
    printf("remove all"); while (vec->size(vec) != 0) { vec->removelast(vec); }
    printf(", size=%zu\n", vec->size(vec));
    printf("addlast: &a"); vec->addlast(vec, &a);
    printf(", &b"); vec->addlast(vec, &b);
    printf(", &c"); vec->addlast(vec, &c);
    printf(", &d"); vec->addlast(vec, &d);
    printf(", &e"); vec->addlast(vec, &e);
    printf(", size=%zu\n", vec->size(vec));
    vec->free(vec);
}

void fails_with_double_free() {
    printf("\nfails_with_double_free:\n");
    qvector_t *vec = qvector(10, sizeof(uint64_t), QVECTOR_RESIZE_DOUBLE);
    uint64_t a = 1, b = 2, c = 3, d = 4, e = 5;
    printf("addlast: &a"); vec->addlast(vec, &a);
    printf(", &b"); vec->addlast(vec, &b);
    printf(", &c"); vec->addlast(vec, &c);
    printf(", &d"); vec->addlast(vec, &d);
    printf(", &e"); vec->addlast(vec, &e);
    printf(", size=%zu\n", vec->size(vec));
    printf("clear"); vec->clear(vec);
    printf(", size=%zu\n", vec->size(vec));
    printf("addlast: &a"); vec->addlast(vec, &a);
    printf(", &b"); vec->addlast(vec, &b);
    printf(", &c"); vec->addlast(vec, &c);
    printf(", &d"); vec->addlast(vec, &d);
    printf(", &e"); vec->addlast(vec, &e);
    printf(", size=%zu\n", vec->size(vec));
    vec->free(vec);
}

void also_fails_with_double_free() {
    printf("\nalso_fails_with_double_free:\n");
    qvector_t *vec = qvector(10, sizeof(uint64_t), QVECTOR_RESIZE_DOUBLE);
    uint64_t a = 1, b = 2, c = 3, d = 4, e = 5;
    printf("clear"); vec->clear(vec);
    printf(", size=%zu\n", vec->size(vec));
    printf("addlast: &a"); vec->addlast(vec, &a);
    printf(", &b"); vec->addlast(vec, &b);
    printf(", &c"); vec->addlast(vec, &c);
    printf(", &d"); vec->addlast(vec, &d);
    printf(", &e"); vec->addlast(vec, &e);
    printf(", size=%zu\n", vec->size(vec));
    vec->free(vec);
}

int main(int argc, char *argv[]) {
    setbuf(stdout, NULL);
    succeeds();
    fails_with_double_free();
    //also_fails_with_double_free();
    return 0;
}

Output:

succeeds:
addlast: &a, &b, &c, &d, &e, size=5
remove all, size=0
addlast: &a, &b, &c, &d, &e, size=5

fails_with_double_free:
addlast: &a, &b, &c, &d, &e, size=5
clear, size=0
addlast: &a, &b, &c, &dfree(): double free detected in tcache 2

Process finished with exit code 134

Other output:

also_fails_with_double_free:
clear, size=0
addlast: &a, &b, &c, &dfree(): double free detected in tcache 2

I'm statically linking to qlibc and using gcc version:

gcc version 9.3.0 (Ubuntu 9.3.0-10ubuntu2)

Does this look like an issue or am I doing something wrong? My workaround for now is to wrap "remove all" into a function; this will work fine for me since my use case isn't speed-critical.

[wolkykim]

Hi, it looks like it's a bug. I'm looking into it. Thanks for the report.

[wolkykim]

Can you review https://github.com/wolkykim/qlibc/pull/73 and confirm if the change fix the problems from the branch https://github.com/wolkykim/qlibc/tree/vector_fix?

[bryanwagner]

Checked out the branch and it's now fixed! My test program works and my original use case (more things going on) also now works.

New output:

succeeds:
addlast: &a, &b, &c, &d, &e, size=5
remove all, size=0
addlast: &a, &b, &c, &d, &e, size=5

fails_with_double_free (fixed!):
addlast: &a, &b, &c, &d, &e, size=5
clear, size=0
addlast: &a, &b, &c, &d, &e, size=5

also_fails_with_double_free (fixed!):
clear, size=0
addlast: &a, &b, &c, &d, &e, size=5

Process finished with exit code 0

I agree with the diff in branch vector_fix. It makes sense to me that you would keep data and its metadata the same and simply reset num. In my original case, I'm moving the data out of the vector before calling clear, which should be expected of the user.

I checked for consistency and the remove methods don't clear memory to zero (which I do agree with), so I think the change is correct.

Side note: briefly looking at remove_at you may be able to replace the for loop with a single call to memcpy since vector->objsize is fixed: shift vector->objsize * (vector->num - (index + 1)) bytes.

Thanks for fixing this so quickly!

[wolkykim]

thanks for the confirmation. will merge in and release soon.

you may be able to replace the for loop with a single call to memcpy since vector->objsize is fixed: shift vector->objsize * (vector->num - (index + 1)) bytes.

I get what you're saying and I know gnu libc works that way but wasn't sure if it will be fine with other compilers as the there's overlap in the source and destination. If you're sure about that, you can create a PR then request a merge to me.

[bryanwagner]

Sounds great! I wouldn't worry about that last optimization. It's working fine as-is and I would err on the side of caution in this case. (And doing a quick lookup it would need to change to memmove anyway - I would not change it)