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

[Vladimir Homutov]

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:


-------------- next part --------------
A non-text attachment was scrubbed...
Name: request_checks.diff
Type: text/x-diff
Size: 2485 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20231129/b2386583/attachment.bin>