Welcome! Log In Create A New Profile

Advanced

Re: [PATCH 2 of 2] HTTP: suppressed possible overflow in interim r->uri_end calculation

Maxim Dounin
October 27, 2023 02:52PM
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
Subject Author Views Posted

[PATCH 0 of 2] [patch] some issues found by gcc undef sanitizer

Vladimir Homutov via nginx-devel 400 October 27, 2023 08:00AM

[PATCH 2 of 2] HTTP: suppressed possible overflow in interim r->uri_end calculation

Vladimir Homutov via nginx-devel 59 October 27, 2023 08:00AM

Re: [PATCH 2 of 2] HTTP: suppressed possible overflow in interim r->uri_end calculation

Maxim Dounin 74 October 27, 2023 02:52PM

[PATCH 0 of 2] [patch] some issues found by gcc undef sanitizer

Vladimir Homutov via nginx-devel 51 November 10, 2023 04:14AM

[PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer

Vladimir Homutov via nginx-devel 71 November 10, 2023 04:14AM

Re: [PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer

Maxim Dounin 44 November 27, 2023 10:00PM

Re: [PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer

Vladimir Homutov via nginx-devel 41 November 29, 2023 03:24AM

Re: [PATCH 1 of 2] HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer

Maxim Dounin 43 November 29, 2023 08:16PM

[PATCH 1 of 2] Core: avoid calling memcpy() in edge cases

Vladimir Homutov via nginx-devel 61 October 27, 2023 08:00AM

Re: [PATCH 1 of 2] Core: avoid calling memcpy() in edge cases

Maxim Dounin 51 December 03, 2023 10:18PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 243
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready