Maxim Dounin
November 27, 2023 10:00PM
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...".

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.

>
> 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) {

See review of the second patch about r->port_start / r->port_end.
I would rather change it similarly for now.

> @@ -1754,9 +1763,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.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
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 568 October 27, 2023 08:00AM

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

Vladimir Homutov via nginx-devel 146 October 27, 2023 08:00AM

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

Maxim Dounin 147 October 27, 2023 02:52PM

[PATCH 0 of 2] [patch] some issues found by gcc undef sanitizer

Vladimir Homutov via nginx-devel 121 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 152 November 10, 2023 04:14AM

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

Maxim Dounin 148 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 122 November 29, 2023 03:24AM

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

Maxim Dounin 133 November 29, 2023 08:16PM

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

Vladimir Homutov via nginx-devel 137 October 27, 2023 08:00AM

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

Maxim Dounin 127 December 03, 2023 10:18PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 201
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 500 on July 15, 2024
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready