Welcome! Log In Create A New Profile

Advanced

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

Vladimir Homutov via nginx-devel
November 29, 2023 03:24AM
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@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:


_______________________________________________
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 60 October 27, 2023 08:00AM

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

Maxim Dounin 75 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 44 November 29, 2023 08:16PM

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

Vladimir Homutov via nginx-devel 62 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: 133
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