[stojkomilos]

kll_sketch::update(item) copies item into the sketch after internal_update(). If item refers to an item of the same sketch compaction is triggered, those items move. This then leads to item becoming invalid in the middle of the call kll_sketch::update(item).

Note: on master, this is triggered only by compaction, but after #569 merged, it will also be triggered by regrowth. This is not too important for understanding this PR, as the fix is identical for both situations.

Master

This is @proost 's test from #569 (see the next section), with k = 200 instead of 5000, so that the 201st update triggers the first compaction:

TEST_CASE("kll sketch: update with aliased item during compaction", "[kll_sketch]") {
  const uint16_t k = 200;
  kll_sketch<std::string> sketch(k);

  for (int i = 0; i < kll_constants::DEFAULT_K; ++i) {
    sketch.update(std::string(64, 'x') + std::to_string(i));
  }

  REQUIRE(sketch.get_n() == kll_constants::DEFAULT_K);

  const std::string& item = sketch.begin()->first;
  const std::string expected = item;

  // Triggers compaction
  sketch.update(item);

  REQUIRE(sketch.get_n() == kll_constants::DEFAULT_K + 1);

  uint32_t count = 0;
  for (const auto& entry : sketch) {
    if (entry.first == expected) {
      ++count;
    }
  }
  REQUIRE(count == 2);
}

On master (98c6b74), with GCC 10.5 and 10 runs per build:

• -O3: fails 10/10 with std::bad_alloc.
• -O1 -fsanitize=address: heap-use-after-free 10/10. The read is in update() at kll_sketch_impl.hpp:185. The memory was freed in add_empty_top_level_to_completely_full_sketch() (:742), called from compress_while_updating() (:662) and internal_update() (:202).

sketch.update(sketch.get_quantile(0.5)) on a kll_sketch<std::string> hits the same bug, because get_quantile() returns a reference into the sketch. On master it reports heap-use-after-free under ASan. With this PR, the added item equals the median.

Tests

Added 2 tests that would catch this bug. One is for regrowth and one is for compaction.

[stojkomilos]

Compaction and regrowth can only happen when level 0 is full. So we use that fact to handle this situation.

[leerho]

Merge order for the open KLL PRs: #573 first, then #558, then #569. #569's buffer growth depends on this fix, and #558 and #569 change the same storage code, so landing them in that order keeps each rebase small. This one goes first once CI passes and it's approved.