Skip to content

Commit

Permalink
Fix double free in roaring64 andnot_inplace (#556)
Browse files Browse the repository at this point in the history 
* Fix double free in roaring64 andnot_inplace

container_iandnot frees the original container if necessary. Interestingly, as
far as I can tell container_iand does not. Added a test for both.

* Document that container_{ixor, iandnot} free the original

In contrast to other in-place functions, which don't free the original if a
different type of container is created.
  • Loading branch information
@SLieve
SLieve authored Jan 14, 2024
1 parent df5740a commit 4a7474e
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 45 deletions.
16 changes: 8 additions & 8 deletions include/roaring/containers/containers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1728,10 +1728,10 @@ static inline container_t *container_lazy_xor(
* If the returned pointer is identical to c1, then the container has been
* modified.
* If the returned pointer is different from c1, then a new container has been
* created and the caller is responsible for freeing it.
* The type of the first container may change. Returns the modified
* (and possibly new) container
*/
* created. The original container is freed by container_ixor.
* The type of the first container may change. Returns the modified (and
* possibly new) container.
*/
static inline container_t *container_ixor(
container_t *c1, uint8_t type1,
const container_t *c2, uint8_t type2,
Expand Down Expand Up @@ -1960,10 +1960,10 @@ static inline container_t *container_andnot(
* If the returned pointer is identical to c1, then the container has been
* modified.
* If the returned pointer is different from c1, then a new container has been
* created and the caller is responsible for freeing it.
* The type of the first container may change. Returns the modified
* (and possibly new) container
*/
* created. The original container is freed by container_iandnot.
* The type of the first container may change. Returns the modified (and
* possibly new) container.
*/
static inline container_t *container_iandnot(
container_t *c1, uint8_t type1,
const container_t *c2, uint8_t type2,
Expand Down
7 changes: 6 additions & 1 deletion src/roaring64.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1282,13 +1282,18 @@ void roaring64_bitmap_andnot_inplace(roaring64_bitmap_t *r1,
container2 = container_andnot(
leaf1->container, leaf1->typecode, leaf2->container,
leaf2->typecode, &typecode2);
if (container2 != container1) {
// We only free when doing container_andnot, not
// container_iandnot, as iandnot frees the original
// internally.
container_free(container1, typecode1);
}
} else {
container2 = container_iandnot(
leaf1->container, leaf1->typecode, leaf2->container,
leaf2->typecode, &typecode2);
}
if (container2 != container1) {
container_free(container1, typecode1);
leaf1->container = container2;
leaf1->typecode = typecode2;
}
Expand Down
98 changes: 62 additions & 36 deletions tests/roaring64_unit.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -626,30 +626,43 @@ DEFINE_TEST(test_and_cardinality) {
}

DEFINE_TEST(test_and_inplace) {
roaring64_bitmap_t* r1 = roaring64_bitmap_create();
roaring64_bitmap_t* r2 = roaring64_bitmap_create();
{
roaring64_bitmap_t* r1 = roaring64_bitmap_create();
roaring64_bitmap_t* r2 = roaring64_bitmap_create();

roaring64_bitmap_add(r1, 50000);
roaring64_bitmap_add(r1, 100000);
roaring64_bitmap_add(r1, 100001);
roaring64_bitmap_add(r1, 200000);
roaring64_bitmap_add(r1, 300000);
roaring64_bitmap_add(r1, 50000);
roaring64_bitmap_add(r1, 100000);
roaring64_bitmap_add(r1, 100001);
roaring64_bitmap_add(r1, 200000);
roaring64_bitmap_add(r1, 300000);

roaring64_bitmap_add(r2, 100001);
roaring64_bitmap_add(r2, 200000);
roaring64_bitmap_add(r2, 400000);
roaring64_bitmap_add(r2, 100001);
roaring64_bitmap_add(r2, 200000);
roaring64_bitmap_add(r2, 400000);

roaring64_bitmap_and_inplace(r1, r2);
roaring64_bitmap_and_inplace(r1, r2);

assert_false(roaring64_bitmap_contains(r1, 50000));
assert_false(roaring64_bitmap_contains(r1, 100000));
assert_true(roaring64_bitmap_contains(r1, 100001));
assert_true(roaring64_bitmap_contains(r1, 200000));
assert_false(roaring64_bitmap_contains(r1, 300000));
assert_false(roaring64_bitmap_contains(r1, 400000));
assert_false(roaring64_bitmap_contains(r1, 50000));
assert_false(roaring64_bitmap_contains(r1, 100000));
assert_true(roaring64_bitmap_contains(r1, 100001));
assert_true(roaring64_bitmap_contains(r1, 200000));
assert_false(roaring64_bitmap_contains(r1, 300000));
assert_false(roaring64_bitmap_contains(r1, 400000));

roaring64_bitmap_free(r1);
roaring64_bitmap_free(r2);
roaring64_bitmap_free(r1);
roaring64_bitmap_free(r2);
}
{
// No intersection.
roaring64_bitmap_t* r1 = roaring64_bitmap_from_range(0, 100, 1);
roaring64_bitmap_t* r2 = roaring64_bitmap_from_range(100, 200, 1);

roaring64_bitmap_and_inplace(r1, r2);
assert_true(roaring64_bitmap_is_empty(r1));

roaring64_bitmap_free(r1);
roaring64_bitmap_free(r2);
}
}

DEFINE_TEST(test_intersect) {
Expand Down Expand Up @@ -878,28 +891,41 @@ DEFINE_TEST(test_andnot_cardinality) {
}

DEFINE_TEST(test_andnot_inplace) {
roaring64_bitmap_t* r1 = roaring64_bitmap_create();
roaring64_bitmap_t* r2 = roaring64_bitmap_create();
{
roaring64_bitmap_t* r1 = roaring64_bitmap_create();
roaring64_bitmap_t* r2 = roaring64_bitmap_create();

roaring64_bitmap_add(r1, 100000);
roaring64_bitmap_add(r1, 100001);
roaring64_bitmap_add(r1, 200000);
roaring64_bitmap_add(r1, 300000);
roaring64_bitmap_add(r1, 100000);
roaring64_bitmap_add(r1, 100001);
roaring64_bitmap_add(r1, 200000);
roaring64_bitmap_add(r1, 300000);

roaring64_bitmap_add(r2, 100001);
roaring64_bitmap_add(r2, 200000);
roaring64_bitmap_add(r2, 400000);
roaring64_bitmap_add(r2, 100001);
roaring64_bitmap_add(r2, 200000);
roaring64_bitmap_add(r2, 400000);

roaring64_bitmap_andnot_inplace(r1, r2);
roaring64_bitmap_andnot_inplace(r1, r2);

assert_true(roaring64_bitmap_contains(r1, 100000));
assert_false(roaring64_bitmap_contains(r1, 100001));
assert_false(roaring64_bitmap_contains(r1, 200000));
assert_true(roaring64_bitmap_contains(r1, 300000));
assert_false(roaring64_bitmap_contains(r1, 400000));
assert_true(roaring64_bitmap_contains(r1, 100000));
assert_false(roaring64_bitmap_contains(r1, 100001));
assert_false(roaring64_bitmap_contains(r1, 200000));
assert_true(roaring64_bitmap_contains(r1, 300000));
assert_false(roaring64_bitmap_contains(r1, 400000));

roaring64_bitmap_free(r1);
roaring64_bitmap_free(r2);
roaring64_bitmap_free(r1);
roaring64_bitmap_free(r2);
}
{
// Two identical bitmaps.
roaring64_bitmap_t* r1 = roaring64_bitmap_from_range(0, 100, 1);
roaring64_bitmap_t* r2 = roaring64_bitmap_from_range(0, 100, 1);

roaring64_bitmap_andnot_inplace(r1, r2);
assert_true(roaring64_bitmap_is_empty(r1));

roaring64_bitmap_free(r1);
roaring64_bitmap_free(r2);
}
}

bool roaring_iterator64_sumall(uint64_t value, void* param) {
Expand Down

0 comments on commit 4a7474e

Please sign in to comment.