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