Repository navigation
tests: a QUIC engine is disposed after the reactors serving it have stopped - #394
Merged
Merged
Conversation
…topped E2E crashed with exit 139 in about a third of CI runs since #388, the #392 release merge among them, so 0.20.392 was never packed or published. Tests wrote `using var engine = new QuicEngine(...)`, which disposed the engine where the test body ended, before the runner stopped the server. The reactor's teardown then freed each live connection through ptls_free, which reads the TLS context the engine had already freed. It crashed only when that memory had been reused. Under glibc's malloc perturb it crashes on every run, on main and on a build from before #388 alike. TestServer.Own hands an engine to the harness, which disposes it in StopAll once the reactors have joined. The 145 sites that disposed theirs in the test body now use it.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
E2E on main has crashed with exit 139 in 3 of the 9 runs since #388: the #388 merge, the release merge (#392, so 0.20.392 was never packed or published), and #393. The use-after-free is in the tests, not the library.
A test that writes
using var engine = new QuicEngine(...)disposes the engine where its body ends. The runner only stops the server after that, in itsfinally. Reactor teardown then frees each live connection, andptls_freereads the TLS context the engine already freed (update_open_count(tls->ctx, -1)).QuicEngine.Disposealready says the engine must outlive its connections. Three test files did that by hand, stopping the server and then disposing; 145 sites did not.It crashed only when the freed block had been reused. glibc's malloc perturb (
GLIBC_TUNABLES=glibc.malloc.perturb=165) fills freed memory with0xa5. Under it, main crashes on every run, at the first QUIC test whose server outlives its engine. The faulting instruction iscall *(%rax)inptls_freewithrax = 0xa5a5a5a5a5a5a5a5, underiq_conn_free ← QuicEngineConnection.Teardown ← Reactor.TeardownQuic. A build from before #388 crashes the same way, so #388 only made the reuse likelier.TestServer.Ownhands an engine to the harness, which disposes it inStopAllonce the reactors have joined. If a reactor does not stop, its engines are left to process exit with it. The 145 sites now useOwn:using var;H3LargeResponseTests.Connect, which no longer hands its engine out;using (engine)block inH3AlpnTests.Tests only; no library change.
All eight suites pass on this branch: E2E 326, Unit 105, Http 89, Tls 194, Chaos 80, File 5, Pg 18 and Redis 6, plus Redis's known pending test (#336). They also pass under malloc perturb, which unfixed main cannot get through.
Once this merges, the main run publishes 0.20.392: the version is unchanged, and the publish step skips duplicates.