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@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@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel