Hello! On Mon, Aug 26, 2024 at 12:14:48PM +0200, Jiří Setnička via nginx-devel wrote:
> Hello, > > I believe that I encountered segfault caused by this change: > > details: http://freenginx.org/hg/nginx/rev/81082b5521dd > > branches: > > changeset: 9259:81082b5521dd > > user: Maxim Dounin <mdou...@mdounin.ru> > > date: Sat Apr 27 18:21:38 2024 +0300 > > description: > > Request body: body is now cleared on errors. > > > > Previously, after errors the request body was left in a potentially > > inconsistent state, with r->headers_in.content_length_n which might be > > larger than buffers actually stored in r->request_body->bufs (or not > > set at all, in case of HTTP/2 and HTTP/3). This can cause issues if > > the request body is subsequently used during error_page handling, such > > as when proxying. > > > > Fix is to clear r->request_body->bufs if this happens, and set > > r->headers_in.content_length_n to 0, much like it happens when > > ngx_http_discard_request_body() is called when returning 413 from > > ngx_http_core_find_config_phase() for requests with Content-Length. > > > > ... > > > > diff --git a/src/http/ngx_http_request_body.c > > b/src/http/ngx_http_request_body.c > > --- a/src/http/ngx_http_request_body.c > > +++ b/src/http/ngx_http_request_body.c > > @@ -228,6 +228,11 @@ done: > > } > > if (rc >= NGX_HTTP_SPECIAL_RESPONSE) { > > + > > + r->lingering_close = 1; > > + r->headers_in.content_length_n = 0; > > + r->request_body->bufs = NULL; > > + > > r->main->count--; > > r->read_event_handler = ngx_http_block_reading; > > } > > The r->request_body may not be set here. It could happen at the top of the > ngx_http_read_client_request_body function, when either ngx_http_test_expect > or ngx_pcalloc fails and the goto done is executed before setting > r->request_body. > > I encountered this with clients that sends Expect: 100-continue but > immediately closes the connection. > > The fix should be straightforward, just to check for existence of the > r->request_body. Thanks for reporting this. Indeed, clearly there is a bug (and I'm able to reproduce it with "Expect: 100-continue"). Here is a fix: # HG changeset patch # User Maxim Dounin <mdou...@mdounin.ru> # Date 1724686663 -10800 # Mon Aug 26 18:37:43 2024 +0300 # Node ID a507fb4679a4578077d301c723855b5a6e54d4eb # Parent d6f75dd66761c10d4bfb257ae70a212411b6a69b Request body: fixed segfault on early errors. The r->request_body might not be initialized on error handling in ngx_http_read_client_request_body(), notably if ngx_http_test_expect() or ngx_pcalloc() fail. After introduction of request body clearing in 9259:81082b5521dd (1.27.0), this caused segmentation fault due to NULL pointer dereference when clearing r->request_body->bufs. Fix is to explicitly check if r->request_body is available before clearing r->request_body->bufs. Reported by Jiří Setnička, http://freenginx.org/pipermail/nginx-devel/2024-August/000484.html diff --git a/src/http/ngx_http_request_body.c b/src/http/ngx_http_request_body.c --- a/src/http/ngx_http_request_body.c +++ b/src/http/ngx_http_request_body.c @@ -245,7 +245,10 @@ done: r->lingering_close = 1; r->discard_body = 1; - r->request_body->bufs = NULL; + + if (r->request_body) { + r->request_body->bufs = NULL; + } r->main->count--; r->read_event_handler = ngx_http_block_reading; -- Maxim Dounin http://mdounin.ru/
