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

[Maxim Dounin]

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...".

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/