Hello! On Fri, Aug 08, 2025 at 11:09:03PM +0300, Maxim Dounin wrote:
> # HG changeset patch > # User Maxim Dounin <[email protected]> > # Date 1754683258 -10800 > # Fri Aug 08 23:00:58 2025 +0300 > # Node ID 9c5a58441dc9fd62280681e7f92b74b4e69e5104 > # Parent 081c23342274d57259786c115dd55c08eb03ac9e > Reworked ngx_http_parse_status_line() to avoid data assumptions. > > With this change, ngx_http_parse_status_line() does not assume anything > about ngx_http_status_t initial values, and does not need it to be cleared > on allocation or during reinitialization. Further, status->count is > no longer used at all, separate states are used instead. > > Additionally, status digits parsing now does not permit spaces between > digits, which previously were allowed, yet resulted in incorrect status > line sent to the client. > > diff --git a/src/http/modules/ngx_http_proxy_module.c > b/src/http/modules/ngx_http_proxy_module.c > --- a/src/http/modules/ngx_http_proxy_module.c > +++ b/src/http/modules/ngx_http_proxy_module.c > @@ -1628,10 +1628,6 @@ ngx_http_proxy_reinit_request(ngx_http_r > return NGX_OK; > } > > - ctx->status.code = 0; > - ctx->status.count = 0; > - ctx->status.start = NULL; > - ctx->status.end = NULL; > ctx->chunked.state = 0; > > r->upstream->process_header = ngx_http_proxy_process_status_line; > diff --git a/src/http/modules/ngx_http_scgi_module.c > b/src/http/modules/ngx_http_scgi_module.c > --- a/src/http/modules/ngx_http_scgi_module.c > +++ b/src/http/modules/ngx_http_scgi_module.c > @@ -489,7 +489,7 @@ ngx_http_scgi_handler(ngx_http_request_t > return NGX_HTTP_INTERNAL_SERVER_ERROR; > } > > - status = ngx_pcalloc(r->pool, sizeof(ngx_http_status_t)); > + status = ngx_palloc(r->pool, sizeof(ngx_http_status_t)); > if (status == NULL) { > return NGX_HTTP_INTERNAL_SERVER_ERROR; > } > @@ -985,19 +985,6 @@ ngx_http_scgi_create_request(ngx_http_re > static ngx_int_t > ngx_http_scgi_reinit_request(ngx_http_request_t *r) > { > - ngx_http_status_t *status; > - > - status = ngx_http_get_module_ctx(r, ngx_http_scgi_module); > - > - if (status == NULL) { > - return NGX_OK; > - } > - > - status->code = 0; > - status->count = 0; > - status->start = NULL; > - status->end = NULL; > - > r->upstream->process_header = ngx_http_scgi_process_status_line; > r->state = 0; > > diff --git a/src/http/modules/ngx_http_uwsgi_module.c > b/src/http/modules/ngx_http_uwsgi_module.c > --- a/src/http/modules/ngx_http_uwsgi_module.c > +++ b/src/http/modules/ngx_http_uwsgi_module.c > @@ -657,7 +657,7 @@ ngx_http_uwsgi_handler(ngx_http_request_ > return NGX_HTTP_INTERNAL_SERVER_ERROR; > } > > - status = ngx_pcalloc(r->pool, sizeof(ngx_http_status_t)); > + status = ngx_palloc(r->pool, sizeof(ngx_http_status_t)); > if (status == NULL) { > return NGX_HTTP_INTERNAL_SERVER_ERROR; > } > @@ -1214,19 +1214,6 @@ ngx_http_uwsgi_create_request(ngx_http_r > static ngx_int_t > ngx_http_uwsgi_reinit_request(ngx_http_request_t *r) > { > - ngx_http_status_t *status; > - > - status = ngx_http_get_module_ctx(r, ngx_http_uwsgi_module); > - > - if (status == NULL) { > - return NGX_OK; > - } > - > - status->code = 0; > - status->count = 0; > - status->start = NULL; > - status->end = NULL; > - > r->upstream->process_header = ngx_http_uwsgi_process_status_line; > r->state = 0; > > diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h > --- a/src/http/ngx_http.h > +++ b/src/http/ngx_http.h > @@ -72,7 +72,6 @@ struct ngx_http_chunked_s { > typedef struct { > ngx_uint_t http_version; > ngx_uint_t code; > - ngx_uint_t count; > u_char *start; > u_char *end; > } ngx_http_status_t; > diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c > --- a/src/http/ngx_http_parse.c > +++ b/src/http/ngx_http_parse.c > @@ -1645,7 +1645,9 @@ ngx_http_parse_status_line(ngx_http_requ > sw_major_digit, > sw_first_minor_digit, > sw_minor_digit, > - sw_status, > + sw_first_status_digit, > + sw_second_status_digit, > + sw_third_status_digit, > sw_space_after_status, > sw_status_text, > sw_almost_done > @@ -1750,7 +1752,7 @@ ngx_http_parse_status_line(ngx_http_requ > /* the minor HTTP version or the end of the request line */ > case sw_minor_digit: > if (ch == ' ') { > - state = sw_status; > + state = sw_first_status_digit; > break; > } > > @@ -1765,8 +1767,8 @@ ngx_http_parse_status_line(ngx_http_requ > r->http_minor = r->http_minor * 10 + (ch - '0'); > break; > > - /* HTTP status code */ > - case sw_status: > + /* the first digit of HTTP status code */ > + case sw_first_status_digit: > if (ch == ' ') { > break; > } > @@ -1775,13 +1777,29 @@ ngx_http_parse_status_line(ngx_http_requ > return NGX_ERROR; > } > > + status->code = ch - '0'; > + status->start = p; > + state = sw_second_status_digit; > + break; > + > + /* the second digit of HTTP status code */ > + case sw_second_status_digit: > + if (ch < '0' || ch > '9') { > + return NGX_ERROR; > + } > + > status->code = status->code * 10 + (ch - '0'); > - > - if (++status->count == 3) { > - state = sw_space_after_status; > - status->start = p - 2; > + state = sw_third_status_digit; > + break; > + > + /* the third digit of HTTP status code */ > + case sw_third_status_digit: > + if (ch < '0' || ch > '9') { > + return NGX_ERROR; > } > > + status->code = status->code * 10 + (ch - '0'); > + state = sw_space_after_status; > break; > > /* space or end of line */ > @@ -1810,6 +1828,7 @@ ngx_http_parse_status_line(ngx_http_requ > state = sw_almost_done; > break; > case LF: > + status->end = p; > goto done; > } > break; > @@ -1835,10 +1854,6 @@ done: > > b->pos = p + 1; > > - if (status->end == NULL) { > - status->end = p; > - } > - > status->http_version = r->http_major * 1000 + r->http_minor; > r->state = sw_start; > > And yet another "status->end" change, missed in the previous patch: @@ -1797,6 +1815,7 @@ ngx_http_parse_status_line(ngx_http_requ state = sw_almost_done; break; case LF: + status->end = p; goto done; default: return NGX_ERROR; And corresponding test: diff --git a/proxy_status.t b/proxy_status.t --- a/proxy_status.t +++ b/proxy_status.t @@ -57,7 +57,7 @@ http { EOF $t->run_daemon(\&http_daemon); -$t->try_run('no proxy_allow_http09')->plan(12); +$t->try_run('no proxy_allow_http09')->plan(13); $t->waitforsocket('127.0.0.1:' . port(8081)); ############################################################################### $ hg qref $ hg qdiff diff --git a/proxy_status.t b/proxy_status.t --- a/proxy_status.t +++ b/proxy_status.t @@ -10,7 +10,7 @@ use warnings; use strict; use Test::More; -use Socket qw/ CRLF /; +use Socket qw/ CRLF LF /; BEGIN { use FindBin; chdir($FindBin::Bin); } @@ -57,7 +57,7 @@ http { EOF $t->run_daemon(\&http_daemon); -$t->try_run('no proxy_allow_http09')->plan(12); +$t->try_run('no proxy_allow_http09')->plan(13); $t->waitforsocket('127.0.0.1:' . port(8081)); ############################################################################### @@ -68,6 +68,11 @@ like(http_get('/600'), qr!^HTTP/1.1 600 like(http_get('/http10'), qr!^HTTP/1.1 200 !s, 'http 1.0 200'); like(http_get('/duplicate'), qr!^HTTP/1.1 200 !s, 'duplicate status ignored'); +# status line without text and trailing space, +# invalid but currently accepted + +like(http_get('/notext'), qr!^HTTP/1.1 200!s, 'status without text'); + # HTTP/0.9 is disabled by default since 1.29.1 like(http_get('/http09'), qr!^HTTP/1.1 502 !s, 'http 0.9'); @@ -142,6 +147,12 @@ sub http_daemon { 'HTTP/1.1 204 No content' . CRLF . 'Connection: close' . CRLF . CRLF; + } elsif ($uri eq '/notext') { + + print $client + 'HTTP/1.1 200' . LF . + 'Connection: close' . CRLF . CRLF; + } elsif ($uri =~ m!/http09!) { print $client 'It is HTTP/0.9 response' . CRLF; -- Maxim Dounin http://mdounin.ru/
