search

canonical / dqlite

Embeddable, replicated and fault-tolerant SQL engine.
https://dqlite.io
Other
3.89k stars 219 forks source link

Fix use-after-free in unit tests #686

Closed cole-miller closed 3 months ago

cole-miller commented 3 months ago

Free the pool's pool_impl_t containing the outq_async handle only after the callback passed to uv_close has fired, in line with libuv rules, and in contrast to the original code, which could free this allocation while libuv was still holding a pointer into it. This UAF showed up in CI runs for #682 but is hard to reproduce otherwise.

Note that dqlite as used in production is not affected by this bug since the thread pool is not even created in that case.

Signed-off-by: Cole Miller cole.miller@canonical.com

cole-miller commented 3 months ago

I suspect this was behind the elusive hangs we were seeing in CI as well...

cole-miller commented 3 months ago

Need to fix pool setup/teardown in server.c as well.

cole-miller commented 3 months ago

I discussed this offline with @just-now, who pointed out that the pool was designed based on the assumption that pool_fini would not be called until the associated event loop had been run to completion. This makes it sound to free the pool_impl_t in pool_fini. I've updated the tests to use pool_fini as intended and documented the thread pool's closing sequence.

However, there is a bigger issue: gateway__leader_close is unsound when using the new thread pool, because it invokes the request callbacks in a way that's not synchronized with operations running on the pool that use the same resources, in particular prepared statements. So I've just disabled this test for now when DQLITE_NEXT is set, with a note about the race condition.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 77.87%. Comparing base (2d0d71b) to head (9387b77). Report is 7 commits behind head on master.

Files Patch % Lines
src/lib/threadpool.c 20.00% 3 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #686 +/- ## ========================================== - Coverage 77.87% 77.87% -0.01% ========================================== Files 197 197 Lines 27953 27955 +2 Branches 5534 5532 -2 ========================================== + Hits 21768 21769 +1 + Misses 4346 4345 -1 - Partials 1839 1841 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.