timer: pad Data so the ngx_event_ident debug read stays in bounds#31
Open
climagabriel wants to merge 1 commit into
Open
timer: pad Data so the ngx_event_ident debug read stays in bounds#31climagabriel wants to merge 1 commit into
climagabriel wants to merge 1 commit into
Conversation
nginx debug logging evaluates ngx_event_ident(ev->data) on every timer expiry, reading ((ngx_connection_t *) data)->fd — 4 bytes at offset 24. Timer sets ev->data to a Box<Data<HandlerFn>> that can be as small as 8 bytes (24 observed), so the read lands past the allocation: benign garbage in a debug line on plain builds, fatal heap-buffer-overflow under AddressSanitizer (worker abort on first expiry on any node with 'error_log memory:32m debug' and a --with-debug binary). Pad Data to keep offset 24..28 readable.
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.
Bug
Timer::startsetsev->datato aBox<Data<HandlerFn>>(observed 24 bytes; can be as small as 8 with a zero-sized handler). nginx's timer machinery assumesev->datapoints at anngx_connection_tfor debug logging:ngx_event_expire_timers(andngx_event_add_timer/ngx_event_del_timer) evaluatengx_event_ident(ev->data)=((ngx_connection_t *) data)->fd— a 4-byte read at offset 24.On a
--with-debugbinary with any debug-level log sink configured (gcore edges all carryerror_log memory:32m debug), that read executes on every timer expiry and lands past theDataallocation:heap-buffer-overflow, worker aborts on the first fastedge timer expiry, master respawns, repeat — a continuous worker crash loopFound by the EPX-151 ASan package variant (
1.30.2-9.epx151.asan.1) on the am3 loadtest edge: ~2500 worker aborts in 4 minutes, all identical.Fix
Pad
Dataso bytes 24..28 of the allocation are always in bounds. Minimal-diff alternative to restructuring the event+data into one#[repr(C)]box; nginx's own non-connectionev->datausers (resolver, cache manager) are safe only because their structs happen to be large enough — this makes fastedge's explicitly so.