Hello!

On Fri, Oct 27, 2023 at 02:58:45PM +0300, Vladimir Homutov via nginx-devel wrote:

> If URI is not fully parsed yet, the r->uri_end pointer is NULL.
> As a result, calculation of "new + (r->uri_end - old)" expression
> may overflow. In such case, just avoid calculating it, as r->uri_end
> will be set correctly later by the parser in any case.
>
> The issue was found by GCC undefined behaviour sanitizer.
>
>
> src/http/ngx_http_request.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
>

> # HG changeset patch
> # User Vladimir Khomutov <vl@wbsrv.ru>
> # Date 1698407686 -10800
> # Fri Oct 27 14:54:46 2023 +0300
> # Node ID 1b28902de1c648fc2586bba8e05c2ff63e0e33cb
> # Parent ef9f124b156aff0e9f66057e438af835bd7a60d2
> HTTP: suppressed possible overflow in interim r->uri_end calculation.
>
> If URI is not fully parsed yet, the r->uri_end pointer is NULL.
> As a result, calculation of "new + (r->uri_end - old)" expression
> may overflow. In such case, just avoid calculating it, as r->uri_end
> will be set correctly later by the parser in any case.
>
> The issue was found by GCC undefined behaviour sanitizer.
>
> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -1721,7 +1721,9 @@ ngx_http_alloc_large_header_buffer(ngx_h
> r->method_end = new + (r->method_end - old);
>
> r->uri_start = new + (r->uri_start - old);
> - r->uri_end = new + (r->uri_end - old);
> + if (r->uri_end) {
> + r->uri_end = new + (r->uri_end - old);
> + }
>
> if (r->schema_start) {
> r->schema_start = new + (r->schema_start - old);

As already noted off-list, this is certainly not the only field
which might be not yet set when
ngx_http_alloc_large_header_buffer() is called. From the patch
context as shown, at least r->method_end and r->uri_start might
not be set as well, leading to similar overflows. And certainly
there are other fields as well.

While I have no objections to fixing such overflows, which formally
might be seen as undefined behaviour (though safe in practice, since
calculated values are never used), I very much object to fixing
just the particular items which were reported by a particular
sanitizer in particular test runs.

Rather, sanitizer results should be used to identify patterns we
want to fix (if at all), and then all such patterns should be
fixed (or not).

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel