[PATCH 08 of 12] Reworked ngx_http_parse_status_line() to avoid data assumptions
Maxim Dounin
mdounin at mdounin.ru
Sun Aug 17 23:33:43 UTC 2025
Hello!
On Fri, Aug 08, 2025 at 11:09:03PM +0300, Maxim Dounin wrote:
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # 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/
More information about the nginx-devel
mailing list