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

[Maxim Dounin]

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 at 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/