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

[Maxim Dounin]

Hello!

On Wed, Nov 29, 2023 at 11:24:03AM +0300, Vladimir Homutov via nginx-devel wrote:

> On Tue, Nov 28, 2023 at 05:58:23AM +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Fri, Nov 10, 2023 at 12:11:54PM +0300, Vladimir Homutov via nginx-devel wrote:
> >
> > > If URI is not fully parsed yet, some pointers are not set.
> > > As a result, the calculation of "new + (ptr - old)" expression
> > > may overflow. In such a case, just avoid calculating it, as value
> > > 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 |  34 ++++++++++++++++++++++++++--------
> > >  1 files changed, 26 insertions(+), 8 deletions(-)
> > >
> > >
> >
> > > # HG changeset patch
> > > # User Vladimir Khomutov <vl at wbsrv.ru>
> > > # Date 1699604478 -10800
> > > #      Fri Nov 10 11:21:18 2023 +0300
> > > # Node ID 505e927eb7a75f0fdce4caddb4ab9d9c71c9b9c8
> > > # Parent  dadd13fdcf5228c8e8380e120d4621002e3b0919
> > > HTTP: uniform overflow checks in ngx_http_alloc_large_header_buffer.
> > >
> > > If URI is not fully parsed yet, some pointers are not set.
> > > As a result, the calculation of "new + (ptr - old)" expression
> > > may overflow. In such a case, just avoid calculating it, as value
> > > will be set correctly later by the parser in any case.
> > >
> > > The issue was found by GCC undefined behaviour sanitizer.
> >
> > I would rather refrain from saying this is an issue, it is not
> > (unless a compiler actually starts to do silly things as long as
> > it sees something formally defined as "undefined behavior" in C
> > standard, and this would be indeed an issue - in the compiler).
> > As already noted in the initial review, the code as written is
> > completely safe in practice.  For such mostly style commits we
> > usually write something like "Prodded by...".
> 
> totally agreed
> 
> >
> > Also note that the issue is not an overflow, but rather
> > subtraction of pointers which do not belong to the same array
> > object (C11, 6.5.6 Additive operators, p.9):
> >
> > : When two pointers are subtracted, both shall point to elements
> > : of the same array object, or one past the last element of the
> > : array object
> >
> > The undefined behaviour is present as long as "ptr" and "old" are
> > not in the same buffer (i.e., array object), which is the case
> > when "ptr" is not set.  And another one follows when trying to add
> > the (already undefined) subtraction result to "new" (since the
> > result is not going to belong to the same array object):
> >
> > : If both the pointer operand and the result point to elements of
> > : the same array object, or one past the last element of the array
> > : object, the evaluation shall not produce an overflow; otherwise,
> > : the behavior is undefined.
> >
> > Overflow here might be an indicator that certainly there is an
> > undefined behaviour, but it's just an indicator.
> >
> > You may want to rewrite commit log accordingly.
> 
> The commit log was updated.
> 
> > > 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
> > > @@ -1718,14 +1718,23 @@ ngx_http_alloc_large_header_buffer(ngx_h
> [...]
> > >          if (r->host_start) {
> >
> > See review of the second patch about r->port_start / r->port_end.
> > I would rather change it similarly for now.
> 
> I would prefer to remove both, so this patch has nothing about it.
> 
> updated patch:
> 
> 

> # HG changeset patch
> # User Vladimir Khomutov <vl at wbsrv.ru>
> # Date 1701245585 -10800
> #      Wed Nov 29 11:13:05 2023 +0300
> # Node ID 7c8ecb3fee20dfbb9a627441377dd09509988e2a
> # Parent  dacad3a9c7b8435a4c67ad2b67f261e7b4e36d8e
> HTTP: uniform checks in ngx_http_alloc_large_header_buffer().
> 
> If URI is not fully parsed yet, some pointers are not set.  As a result,
> the calculation of "new + (ptr - old)" expression is flawed.
> 
> According to  C11, 6.5.6 Additive operators, p.9:

Seems to be an extra space in "to  C11".

> 
> : When two pointers are subtracted, both shall point to elements
> : of the same array object, or one past the last element of the
> : array object
> 
> Since "ptr" is not set, subtraction leads to undefined behaviour, because
> "ptr" and "old" are not in the same buffer (i.e. array objects).
> 
> Prodded 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
> @@ -1718,14 +1718,23 @@ ngx_http_alloc_large_header_buffer(ngx_h
>              r->request_end = new + (r->request_end - old);
>          }
>  
> -        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->method_end) {
> +            r->method_end = new + (r->method_end - old);
> +        }
> +
> +        if (r->uri_start) {
> +            r->uri_start = new + (r->uri_start - 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);
> -            r->schema_end = new + (r->schema_end - old);
> +            if (r->schema_end) {
> +                r->schema_end = new + (r->schema_end - old);
> +            }
>          }
>  
>          if (r->host_start) {
> @@ -1749,9 +1758,18 @@ ngx_http_alloc_large_header_buffer(ngx_h
>  
>      } else {
>          r->header_name_start = new;
> -        r->header_name_end = new + (r->header_name_end - old);
> -        r->header_start = new + (r->header_start - old);
> -        r->header_end = new + (r->header_end - old);
> +
> +        if (r->header_name_end) {
> +            r->header_name_end = new + (r->header_name_end - old);
> +        }
> +
> +        if (r->header_start) {
> +            r->header_start = new + (r->header_start - old);
> +        }
> +
> +        if (r->header_end) {
> +            r->header_end = new + (r->header_end - old);
> +        }
>      }
>  
>      r->header_in = b;

Otherwise looks good, pushed to http://mdounin.ru/hg/nginx/ (with 
the extra space removed), thanks.

-- 
Maxim Dounin
http://mdounin.ru/