Fix use-after-free in unit tests

[cole-miller]

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 [email protected]

[cole-miller]

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

[cole-miller]

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

[cole-miller]

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]

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 ⚠️

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[letFunny]

Just to check my understanding, here we are exploiting the fact that (source):

Handles that wrap file descriptors are closed immediately

So calling uv_close will close the handle and we can free it immediately after. However, we still have to run the event loop to completion prior to doing that because the handles might be in use resulting in UAF.

Is that right?

[cole-miller]

So calling uv_close will close the handle and we can free it immediately after.

No, it won't be closed immediately (uv_async_t doesn't wrap an fd). It'll be closed asynchronously, but we don't free the allocation until after the event loop has exited, which implies that all handles including the pool's async handle have finished closing.