fix: close the connection before destroying its pool - #22
Conversation
ngx_http_multi_upstream_connection_close() destroyed c->pool and then
called ngx_close_connection(c). ngx_close_connection() still logs through
c->log, and for these upstream connections c->log lives in the pool that
was just released, so the debug-level "reusable connection: %ui" from
ngx_reusable_connection() reads a freed ngx_log_t and dereferences a
garbage file descriptor.
Reorder to match ngx_http_close_connection(): mark the connection
destroyed, take the pool aside, close, then destroy the pool. Both callers
in ngx_http_multi_upstream.c treat the close as the final statement, so
nothing observes the connection afterwards.
Reproduces as a worker SIGSEGV on APISIX's t/plugin/dubbo-proxy/upstream.t
TEST 1 with a debug-enabled build:
#0 ngx_write_fd (fd=<error reading variable: Cannot access memory ...>)
#1 ngx_log_error_core (fmt="reusable connection: %ui")
#2 ngx_reusable_connection (c=..., reusable=0)
#3 ngx_close_connection (c=...)
#4 ngx_http_multi_upstream_connection_close (c=...) at
ngx_http_multi_upstream_module.c:755
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 44 minutes Limit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
Problem
ngx_http_multi_upstream_connection_close()destroysc->pooland then callsngx_close_connection(c):ngx_close_connection()still logs throughc->log, and for these upstream connectionsc->loglives in the pool that was just released. On a debug-enabled build thengx_log_debug1(..., "reusable connection: %ui")insidengx_reusable_connection()reads a freedngx_log_tand handsngx_write_fd()a garbage descriptor.Reproduced as a worker SIGSEGV while running APISIX's
t/plugin/dubbo-proxy/upstream.tTEST 1 against apisix-runtime 1.3.16 (roughly one crash per three runs), with this backtrace from the core dump:Solution
Reorder to the sequence nginx itself uses in
ngx_http_close_connection()— mark the connection destroyed, take the pool aside, close, then destroy the pool.ngx_close_connection()never touchesc->pool, and both callers inngx_http_multi_upstream.c(ngx_http_multi_upstream_nextandngx_http_multi_upstream_finalize_request) treat the close as their final statement, so nothing observes the connection after it returns.Verification
The analysis above comes from the core dump of a real crash; the fix itself is code inspection against
ngx_close_connection()and the two call sites. I could not build a faithful apisix-runtime locally to re-run the reproducer against the patched module (the build installs system-wide), so a runtime build would be worth doing before release.