Re: Core: Avoid memcpy from NULL
Hello!
On Wed, Dec 13, 2023 at 11:09:28AM -0500, Ben Kallus wrote:
> Nginx executes numerous `memcpy`s from NULL during normal execution.
> `memcpy`ing to or from NULL is undefined behavior. Accordingly, some
> compilers (gcc -O2) make optimizations that assume `memcpy` arguments
> are not NULL. Nginx with UBSan crashes during startup due to this
> issue.
>
> Consider the following function:
> ```C
> #include
>
> int f(int i) {
> char a[] = {'a'};
> void *src = i ? a : NULL;
> char dst[1];
> memcpy(dst, src, 0);
> return src == NULL;
> }
> ```
> Here's what gcc13.2 -O2 -fno-builtin will do to it:
> ```asm
> f:
> sub rsp, 24
> xor eax, eax
> testedi, edi
> lea rsi, [rsp+14]
> lea rdi, [rsp+15]
> mov BYTE PTR [rsp+14], 97
> cmove rsi, rax
> xor edx, edx
> callmemcpy
> xor eax, eax
> add rsp, 24
> ret
> ```
> Note that `f` always returns 0, regardless of the value of `i`.
>
> Feel free to try for yourself at https://gcc.godbolt.org/z/zfvnMMsds
>
> The reasoning here is that since memcpy from NULL is UB, the optimizer
> is free to assume that `src` is non-null. You might consider this to
> be a problem with the compiler, or the C standard, and I might agree.
> Regardless, relying on UB is inherently un-portable, and requires
> maintenance to ensure that new compiler releases don't break existing
> assumptions about the behavior of undefined operations.
>
> The following patch adds a check to `ngx_memcpy` and `ngx_cpymem` that
> makes 0-length memcpy explicitly a noop. Since all memcpying from NULL
> in Nginx uses n==0, this should be sufficient to avoid UB.
>
> It would be more efficient to instead add a check to every call to
> ngx_memcpy and ngx_cpymem that might be used with src==NULL, but in
> the discussion of a previous patch that proposed such a change, a more
> straightforward and tidy solution was desired.
> It may also be worth considering adding checks for NULL memset,
> memmove, etc. I think this is not necessary unless it is demonstrated
> that Nginx actually executes such undefined calls.
>
> # HG changeset patch
> # User Ben Kallus
> # Date 1702406466 18000
> # Tue Dec 12 13:41:06 2023 -0500
> # Node ID d270203d4ecf77cc14a2652c727e236afc659f4a
> # Parent a6f79f044de58b594563ac03139cd5e2e6a81bdb
> Add NULL check to ngx_memcpy and ngx_cpymem to satisfy UBSan.
>
> diff -r a6f79f044de5 -r d270203d4ecf src/core/ngx_string.c
> --- a/src/core/ngx_string.c Wed Nov 29 10:58:21 2023 +0400
> +++ b/src/core/ngx_string.c Tue Dec 12 13:41:06 2023 -0500
> @@ -2098,6 +2098,10 @@
> ngx_debug_point();
> }
>
> +if (n == 0) {
> +return dst;
> +}
> +
> return memcpy(dst, src, n);
> }
>
> diff -r a6f79f044de5 -r d270203d4ecf src/core/ngx_string.h
> --- a/src/core/ngx_string.h Wed Nov 29 10:58:21 2023 +0400
> +++ b/src/core/ngx_string.h Tue Dec 12 13:41:06 2023 -0500
> @@ -103,8 +103,9 @@
> * gcc3 compiles memcpy(d, s, 4) to the inline "mov"es.
> * icc8 compile memcpy(d, s, 4) to the inline "mov"es or XMM moves.
> */
> -#define ngx_memcpy(dst, src, n) (void) memcpy(dst, src, n)
> -#define ngx_cpymem(dst, src, n) (((u_char *) memcpy(dst, src, n)) + (n))
> +#define ngx_memcpy(dst, src, n) (void) ((n) == 0 ? (dst) : memcpy(dst, src,
> n))
> +#define ngx_cpymem(dst, src, n)
> \
> +((u_char *) ((n) == 0 ? (dst) : memcpy(dst, src, n)) + (n))
>
> #endif
>
> diff -r a6f79f044de5 -r d270203d4ecf src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c Wed Nov 29 10:58:21 2023 +0400
> +++ b/src/http/v2/ngx_http_v2.c Tue Dec 12 13:41:06 2023 -0500
> @@ -3998,9 +3998,7 @@
> n = size;
> }
>
> -if (n > 0) {
> -rb->buf->last = ngx_cpymem(rb->buf->last, pos, n);
> -}
> +rb->buf->last = ngx_cpymem(rb->buf->last, pos, n);
>
> ngx_log_debug1(NGX_LOG_DEBUG_HTTP, fc->log, 0,
> "http2 request body recv %uz", n);
For the record, I've already provided some feedback to Ben in the
ticket here:
https://trac.nginx.org/nginx/ticket/2570
And pointed to the existing thread here:
https://mailman.nginx.org/pipermail/nginx-devel/2023-October/PX7VH5A273NLUGSYC7DR2AZRU75CIQ3Q.html
https://mailman.nginx.org/pipermail/nginx-devel/2023-December/DCGUEGEFS6TSVIWNEWUEZO3FZMR6ESYZ.html
Hope this helps.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Enclosed few thoughts to the subject:
- since it is very rare situation that one needs only a memcpy without
to know whether previous alloc may fail
(e. g. some of pointers were NULL), me too thinks that the caller
should be responsible for the check.
So I would not extend ngx_memcpy or ngx_cpymem in that way.
- rewrite of `ngx_memcpy` define like here:
```
+ #define ngx_memcpy(dst, src, n) (void) ((n) == 0 ? (dst) :
memcpy(dst, src, n))
```
may introduce a regression or compat issues, e. g. fully functioning
codes like that may become broken hereafter:
```
ngx_memcpy(dst, src, ++len); // because n would be incremented twice
in the macro now
```
Sure, `ngx_cpymem` has also the same issue, but it had that already
before the "fix"...
Anyway, I'm always against of such macros (no matter with or without
check it would be better as an inline function instead).
My conclusion:
a fix of affected places invoking `ngx_memcpy` and `ngx_cpymem`, and
possibly an assert in `ngx_memcpy`
and `ngx_cpymem` would be fully enough, in my opinion.
Regards,
Sergey.
On 15.12.2023 03:41, Maxim Dounin wrote:
Hello!
On Wed, Dec 13, 2023 at 11:09:28AM -0500, Ben Kallus wrote:
Nginx executes numerous `memcpy`s from NULL during normal
execution.
`memcpy`ing to or from NULL is undefined behavior. Accordingly,
some
compilers (gcc -O2) make optimizations that assume `memcpy`
arguments
are not NULL. Nginx with UBSan crashes during startup due to this
issue.
Consider the following function:
```C
#include
int f(int i) {
char a[] = {'a'};
void *src = i ? a : NULL;
char dst[1];
memcpy(dst, src, 0);
return src == NULL;
}
```
Here's what gcc13.2 -O2 -fno-builtin will do to it:
```asm
f:
sub rsp, 24
xor eax, eax
testedi, edi
lea rsi, [rsp+14]
lea rdi, [rsp+15]
mov BYTE PTR [rsp+14], 97
cmove rsi, rax
xor edx, edx
callmemcpy
xor eax, eax
add rsp, 24
ret
```
Note that `f` always returns 0, regardless of the value of `i`.
Feel free to try for yourself at
https://gcc.godbolt.org/z/zfvnMMsds
The reasoning here is that since memcpy from NULL is UB, the
optimizer
is free to assume that `src` is non-null. You might consider this
to
be a problem with the compiler, or the C standard, and I might
agree.
Regardless, relying on UB is inherently un-portable, and requires
maintenance to ensure that new compiler releases don't break
existing
assumptions about the behavior of undefined operations.
The following patch adds a check to `ngx_memcpy` and `ngx_cpymem`
that
makes 0-length memcpy explicitly a noop. Since all memcpying from
NULL
in Nginx uses n==0, this should be sufficient to avoid UB.
It would be more efficient to instead add a check to every call to
ngx_memcpy and ngx_cpymem that might be used with src==NULL, but in
the discussion of a previous patch that proposed such a change, a
more
straightforward and tidy solution was desired.
It may also be worth considering adding checks for NULL memset,
memmove, etc. I think this is not necessary unless it is
demonstrated
that Nginx actually executes such undefined calls.
# HG changeset patch
# User Ben Kallus
# Date 1702406466 18000
# Tue Dec 12 13:41:06 2023 -0500
# Node ID d270203d4ecf77cc14a2652c727e236afc659f4a
# Parent a6f79f044de58b594563ac03139cd5e2e6a81bdb
Add NULL check to ngx_memcpy and ngx_cpymem to satisfy UBSan.
diff -r a6f79f044de5 -r d270203d4ecf src/core/ngx_string.c
--- a/src/core/ngx_string.c Wed Nov 29 10:58:21 2023 +0400
+++ b/src/core/ngx_string.c Tue Dec 12 13:41:06 2023 -0500
@@ -2098,6 +2098,10 @@
ngx_debug_point();
}
+if (n == 0) {
+return dst;
+}
+
return memcpy(dst, src, n);
}
diff -r a6f79f044de5 -r d270203d4ecf src/core/ngx_string.h
--- a/src/core/ngx_string.h Wed Nov 29 10:58:21 2023 +0400
+++ b/src/core/ngx_string.h Tue Dec 12 13:41:06 2023 -0500
@@ -103,8 +103,9 @@
* gcc3 compiles memcpy(d, s, 4) to the inline "mov"es.
* icc8 compile memcpy(d, s, 4) to the inline "mov"es or XMM
moves.
*/
-#define ngx_memcpy(dst, src, n) (void) memcpy(dst, src, n)
-#define ngx_cpymem(dst, src, n) (((u_char *) memcpy(dst, src,
n)) + (n))
+#define ngx_memcpy(dst, src, n) (void) ((n) == 0 ? (dst) :
memcpy(dst, src, n))
+#define ngx_cpymem(dst, src, n)
\
+((u_char *) ((n) == 0 ? (dst) : memcpy(dst, src, n)) + (n))
#endif
diff -r a6f79f044de5 -r d270203d4ecf src/http/v2/ngx_http_v2.c
--- a/src/http/v2/ngx_http_v2.
Re: Core: Avoid memcpy from NULL
> - rewrite of `ngx_memcpy` define like here:
> ```
> + #define ngx_memcpy(dst, src, n) (void) ((n) == 0 ? (dst) : memcpy(dst,
> src, n))
> ```
> may introduce a regression or compat issues, e. g. fully functioning codes
> like that may become broken hereafter:
> ```
> ngx_memcpy(dst, src, ++len); // because n would be incremented twice in the
> macro now
> ```
> Sure, `ngx_cpymem` has also the same issue, but it had that already before
> the "fix"...
> Anyway, I'm always against of such macros (no matter with or without check
> it would be better as an inline function instead).
I totally agree. I'm not a fan of function-like macros either; I
introduced those extra evaluations of n and dst only because I saw
that cpymem already valuates n twice. I'd be happy to update these to
functions (explicitly inlined or not) in the final changeset.
> - since it is very rare situation that one needs only a memcpy without to
> know whether previous alloc may fail
> (e. g. some of pointers were NULL), me too thinks that the caller should be
> responsible for the check.
> So I would not extend ngx_memcpy or ngx_cpymem in that way.
I am inclined to agree with you on this point, but Maxim previously wrote this:
> Trying to check length everywhere is just ugly and unreadable.
Which I also think is reasonable.
Does anyone else have a position on this?
-Ben
On 12/15/23, Dipl. Ing. Sergey Brester via nginx-devel
wrote:
> Enclosed few thoughts to the subject:
>
> - since it is very rare situation that one needs only a memcpy without
> to know whether previous alloc may fail
>(e. g. some of pointers were NULL), me too thinks that the caller
> should be responsible for the check.
>So I would not extend ngx_memcpy or ngx_cpymem in that way.
>
> - rewrite of `ngx_memcpy` define like here:
>```
>+ #define ngx_memcpy(dst, src, n) (void) ((n) == 0 ? (dst) :
> memcpy(dst, src, n))
>```
>may introduce a regression or compat issues, e. g. fully functioning
> codes like that may become broken hereafter:
>```
>ngx_memcpy(dst, src, ++len); // because n would be incremented twice
> in the macro now
>```
>Sure, `ngx_cpymem` has also the same issue, but it had that already
> before the "fix"...
>Anyway, I'm always against of such macros (no matter with or without
> check it would be better as an inline function instead).
>
> My conclusion:
>a fix of affected places invoking `ngx_memcpy` and `ngx_cpymem`, and
> possibly an assert in `ngx_memcpy`
>and `ngx_cpymem` would be fully enough, in my opinion.
>
> Regards,
> Sergey.
>
> On 15.12.2023 03:41, Maxim Dounin wrote:
>
>> Hello!
>>
>> On Wed, Dec 13, 2023 at 11:09:28AM -0500, Ben Kallus wrote:
>>
>> Nginx executes numerous `memcpy`s from NULL during normal
>> execution.
>> `memcpy`ing to or from NULL is undefined behavior. Accordingly,
>> some
>> compilers (gcc -O2) make optimizations that assume `memcpy`
>> arguments
>> are not NULL. Nginx with UBSan crashes during startup due to this
>> issue.
>>
>> Consider the following function:
>> ```C
>> #include
>>
>> int f(int i) {
>> char a[] = {'a'};
>> void *src = i ? a : NULL;
>> char dst[1];
>> memcpy(dst, src, 0);
>> return src == NULL;
>> }
>> ```
>> Here's what gcc13.2 -O2 -fno-builtin will do to it:
>> ```asm
>> f:
>> sub rsp, 24
>> xor eax, eax
>> testedi, edi
>> lea rsi, [rsp+14]
>> lea rdi, [rsp+15]
>> mov BYTE PTR [rsp+14], 97
>> cmove rsi, rax
>> xor edx, edx
>> callmemcpy
>> xor eax, eax
>> add rsp, 24
>> ret
>> ```
>> Note that `f` always returns 0, regardless of the value of `i`.
>>
>> Feel free to try for yourself at
>> https://gcc.godbolt.org/z/zfvnMMsds
>>
>> The reasoning here is that since memcpy from NULL is UB, the
>> optimizer
>> is free to assume that `src` is non-null. You might consider this
>> to
>> be a problem with the compiler, or the C standard, and I might
>> agree.
>> Regardless, relying on UB is inherently un-portable, and requires
>> maintenance to ensure that new compiler releases don't break
>> existing
>> assumptions about the behavior of undefined operations.
>>
>> The following patch adds a check to `ngx_memcpy` and `ngx_cpymem`
>> that
>> makes 0-length memcpy explicitly a noop. Since all memcpying from
>> NULL
>> in Nginx uses n==0, this should be sufficient to avoid UB.
>>
>> It would be more efficient to instead add a check to every call to
>> ngx_memcpy and ngx_cpymem that might be used with src==NULL, but in
>> the discussion of a previous patch that proposed such a change, a
>> more
>> straightforward and tidy solution was desired.
>> It may also be worth considering adding
Re: Core: Avoid memcpy from NULL
Hello!
On Fri, Dec 15, 2023 at 03:46:19PM +0100, Dipl. Ing. Sergey Brester via
nginx-devel wrote:
> Enclosed few thoughts to the subject:
>
> - since it is very rare situation that one needs only a memcpy without
> to know whether previous alloc may fail
>(e. g. some of pointers were NULL), me too thinks that the caller
> should be responsible for the check.
>So I would not extend ngx_memcpy or ngx_cpymem in that way.
That's not about failed allocations, it's about ability to work
with empty strings which aren't explicitly set to something other
than { 0, NULL }. And you may refer to Vladimir's patch I've
referenced to find out what it means in terms of the "caller
should be responsible" approach even without trying to look into
places not explicitly reported by the UB sanitizer.
https://mailman.nginx.org/pipermail/nginx-devel/2023-October/PX7VH5A273NLUGSYC7DR2AZRU75CIQ3Q.html
https://mailman.nginx.org/pipermail/nginx-devel/2023-December/DCGUEGEFS6TSVIWNEWUEZO3FZMR6ESYZ.html
> - rewrite of `ngx_memcpy` define like here:
>```
>+ #define ngx_memcpy(dst, src, n) (void) ((n) == 0 ? (dst) :
> memcpy(dst, src, n))
>```
>may introduce a regression or compat issues, e. g. fully functioning
> codes like that may become broken hereafter:
>```
>ngx_memcpy(dst, src, ++len); // because n would be incremented twice
> in the macro now
>```
>Sure, `ngx_cpymem` has also the same issue, but it had that already
> before the "fix"...
>Anyway, I'm always against of such macros (no matter with or without
> check it would be better as an inline function instead).
In general macro definitions in nginx are used everywhere for
efficiency reasons, and macro definitions usually aren't safe.
While some might prefer other approaches, writing code like
"ngx_memcpy(dst, src, ++len)" in nginx is just wrong, and
shouldn't be trusted to work, much like it won't work with
"ngx_cpymem(dst, src, ++len)".
I'm not exactly against using inline functions here, but the
particular argument is at most very weak.
> My conclusion:
>a fix of affected places invoking `ngx_memcpy` and `ngx_cpymem`, and
> possibly an assert in `ngx_memcpy`
>and `ngx_cpymem` would be fully enough, in my opinion.
Well, thank you for your opinion, appreciated. I don't think this
approach is going to work though, see my review of Vladimir's
patch.
Ideally, I would prefer this to be fixed in the C standard (and
GCC). But given this is not a likely option, and there is a
constant stream of reports "hey, UB sanitizer reports about
memcpy(dst, NULL, 0) in nginx" we might consider actually
silencing this by introducing appropriate checks at the interface
level.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
> In general macro definitions in nginx are used everywhere for > efficiency reasons Clang inlines short functions with -O1, and GCC does so with -O2 or -O1 -finline-small-functions. Are there any platforms that Nginx needs to support for which short function inlining isn't sufficient to solve this issue? > While some might prefer other approaches, writing code like > "ngx_memcpy(dst, src, ++len)" in nginx is just wrong, and > shouldn't be trusted to work, much like it won't work with > "ngx_cpymem(dst, src, ++len)". It is indeed wrong to use an expression with a side-effect in an argument to cpymem, but it's also not very obvious that it's wrong. An inlined function solves the argument reevaluation issue without performance overhead. -Ben ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello! On Sat, Dec 16, 2023 at 04:26:37PM -0500, Ben Kallus wrote: > > In general macro definitions in nginx are used everywhere for > > efficiency reasons > > Clang inlines short functions with -O1, and GCC does so with -O2 or > -O1 -finline-small-functions. Are there any platforms that Nginx needs > to support for which short function inlining isn't sufficient to solve > this issue? In nginx, functions expected to be inlined are marked with "ngx_inline", which normally resolves to "inline" (on unix) or "__inline" (on win32). As such, modern versions of both clang and gcc will inline corresponding functions unless optimization is disabled. Still, -O0 is often used at least during development, and it might be unreasonable to introduce extra function calls in basic primitives. Further, nginx generally supports all available platforms reasonably compatible with POSIX and C89. This implies that inline might be not available. > > While some might prefer other approaches, writing code like > > "ngx_memcpy(dst, src, ++len)" in nginx is just wrong, and > > shouldn't be trusted to work, much like it won't work with > > "ngx_cpymem(dst, src, ++len)". > > It is indeed wrong to use an expression with a side-effect in an > argument to cpymem, but it's also not very obvious that it's wrong. An > inlined function solves the argument reevaluation issue without > performance overhead. Sure (but see above about performance overhead; and another question is if it needs to be solved, or following existing style is enough to never see the issue). The point is: in nginx, it's anyway wrong to use arguments with side effects. And even expressions without side effects might won't work. While many macro definitions were adjusted to accept expressions instead of the bare variables (see 2f9214713666 and https://mailman.nginx.org/pipermail/nginx-devel/2020-July/013354.html for an example), some still don't or can be picky. For example, good luck with doing something like "ngx_max(foo & 0xff, bar)". As such, it's certainly not an argument against using checks in macro definitions in the particular patch. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
> Still, -O0 is often used at least during development, and it might > be unreasonable to introduce extra function calls in basic > primitives. I don't think this is a major cause for concern. It is perfectly reasonable for ngx_memcpy be a wrapper function around memcpy; I think most people would assume that from the name. In fact, it's *already* implemented as a function when NGX_MEMCPY_LIMIT is defined. > Further, nginx generally supports all available platforms > reasonably compatible with POSIX and C89. This implies that > inline might be not available. On such platforms, moving ngx_memcpy to a function may introduce some performance overhead. The question is whether slightly better performance on obscure systems is worth the mental overhead of working with function-like macros. > Sure (but see above about performance overhead; and another > question is if it needs to be solved, or following existing style > is enough to never see the issue). A little extra performance overhead on C89 systems, or builds with -O0 is a very small price to pay. ngx_resolver.c:4283 contains direct evidence that function-like macros incur mental overhead: ``` ngx_memcpy(sin6->sin6_addr.s6_addr, addr6[j++].s6_addr, 16); ``` Here we have an expression with a side-effect being passed into a function-like macro. As luck would have it, the second argument to ngx_memcpy is evaluated only once, so this is coincidentally okay. This particular landmine has been armed for a decade (see https://github.com/nginx/nginx/blob/769eded73267274e018f460dd76b417538aa5934/src/core/ngx_resolver.c#L2902). Thus, the existing style guidelines are not enough to prevent issues with function-like macros from arising in nginx. Inline functions solve this problem near-optimally. > good luck with doing something like "ngx_max(foo & 0xff, bar)". Nginx is littered with uses of expressions in ngx_max and ngx_min, it just happens that none of those expressions use operators of lower precedence than < and >. This seems like an invitation for human error. Thus, I argue that the best solution to the memcpy-from-NULL problem is to replace ngx_memcpy and ngx_cpymem with inline functions with the appropriate checks for n==0. Going forward, it's probably smart to consider transitioning away from function-like macros more generally. -Ben ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello! On Fri, Dec 29, 2023 at 04:50:36PM +, Ben Kallus wrote: > > Still, -O0 is often used at least during development, and it might > > be unreasonable to introduce extra function calls in basic > > primitives. > > I don't think this is a major cause for concern. It is perfectly > reasonable for ngx_memcpy be a wrapper function around memcpy; I think > most people would assume that from the name. In fact, it's *already* > implemented as a function when NGX_MEMCPY_LIMIT is defined. The NGX_MEMCPY_LIMIT is a very specific debugging define, which implies additional overhead for memcpy calls. And it's certainly not an excuse for changing all the macro definitions to function calls. Whether or not ngx_memcpy() can be implemented as a wrapper function is a separate question. > > Further, nginx generally supports all available platforms > > reasonably compatible with POSIX and C89. This implies that > > inline might be not available. > > On such platforms, moving ngx_memcpy to a function may introduce some > performance overhead. The question is whether slightly better > performance on obscure systems is worth the mental overhead of working > with function-like macros. As said earlier in the thread, while some might prefer other approaches, function-like macros are used everywhere in nginx. > > Sure (but see above about performance overhead; and another > > question is if it needs to be solved, or following existing style > > is enough to never see the issue). > > A little extra performance overhead on C89 systems, or builds with -O0 > is a very small price to pay. ngx_resolver.c:4283 contains direct > evidence that function-like macros incur mental overhead: > ``` > ngx_memcpy(sin6->sin6_addr.s6_addr, addr6[j++].s6_addr, 16); > ``` > Here we have an expression with a side-effect being passed into a > function-like macro. As luck would have it, the second argument to > ngx_memcpy is evaluated only once, so this is coincidentally okay. > This particular landmine has been armed for a decade (see > https://github.com/nginx/nginx/blob/769eded73267274e018f460dd76b417538aa5934/src/core/ngx_resolver.c#L2902). > Thus, the existing style guidelines are not enough to prevent issues > with function-like macros from arising in nginx. Inline functions > solve this problem near-optimally. The mentioned call specifically assumes that the second argument is only evaluated once, and it was certainly checked that it is only evaluated once when the call was written. That is, there is no issue here. Still, general style guidelines suggests that the code shouldn't be written this way, and the only reason for j++ in the line in question is that it mimics corresponding IPv4 code. > > good luck with doing something like "ngx_max(foo & 0xff, bar)". > > Nginx is littered with uses of expressions in ngx_max and ngx_min, it > just happens that none of those expressions use operators of lower > precedence than < and >. This seems like an invitation for human > error. It's not "just happens". > Thus, I argue that the best solution to the memcpy-from-NULL problem > is to replace ngx_memcpy and ngx_cpymem with inline functions with the > appropriate checks for n==0. Going forward, it's probably smart to > consider transitioning away from function-like macros more generally. As I said earlier in this thread, I'm not exactly against using inline functions here, and I think a solution with inline functions can be accepted provided it is demonstrated that it introduces no measurable performance degradation. Still, I'm very sceptical about the idea of "transitioning away from function-like macros". -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
> Still, general style guidelines suggests that the code shouldn't
> be written this way, and the only reason for j++ in the line in
> question is that it mimics corresponding IPv4 code.
> It's not "just happens".
The point I'm trying to make is that ensuring correctness with
function-like macros is difficult, both because of operator precedence
and argument reevaluation. Expecting contributors to read the
definitions of every macro they use becomes more and more cumbersome
as the codebase expands, especially when some symbols are variably
macros or functions depending on the state of (even infrequently-used)
compile-time constants.
All that said, upon further reflection, I think the UB issue is best
solved outside of ngx_strcpy, where the overhead of an extra check may
have a performance impact. The following patch is sufficient to
silence UBSan in my configuration:
# HG changeset patch
# User Ben Kallus
# Date 1704322684 18000
# Wed Jan 03 17:58:04 2024 -0500
# Node ID 04eb4b1622d1a488f14bb6d5af25e422ff23d82d
# Parent ee40e2b1d0833b46128a357fbc84c6e23be9be07
Add check to ngx_pstrdup to prevent 0-length memcpy.
diff -r ee40e2b1d083 -r 04eb4b1622d1 src/core/ngx_string.c
--- a/src/core/ngx_string.c Mon Dec 25 21:15:48 2023 +0400
+++ b/src/core/ngx_string.c Wed Jan 03 17:58:04 2024 -0500
@@ -77,8 +77,8 @@
u_char *dst;
dst = ngx_pnalloc(pool, src->len);
-if (dst == NULL) {
-return NULL;
+if (dst == NULL || src->len == 0) {
+return dst;
}
ngx_memcpy(dst, src->data, src->len);
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello!
On Wed, Jan 03, 2024 at 11:57:57PM +, Ben Kallus wrote:
> > Still, general style guidelines suggests that the code shouldn't
> > be written this way, and the only reason for j++ in the line in
> > question is that it mimics corresponding IPv4 code.
>
> > It's not "just happens".
>
> The point I'm trying to make is that ensuring correctness with
> function-like macros is difficult, both because of operator precedence
> and argument reevaluation. Expecting contributors to read the
> definitions of every macro they use becomes more and more cumbersome
> as the codebase expands, especially when some symbols are variably
> macros or functions depending on the state of (even infrequently-used)
> compile-time constants.
Sure, and hence the style.
> All that said, upon further reflection, I think the UB issue is best
> solved outside of ngx_strcpy, where the overhead of an extra check may
> have a performance impact. The following patch is sufficient to
> silence UBSan in my configuration:
I've already pointed you to the Vladimir's patch which contains at
least a dozen of places where the same "UB issue" is reported when
running nginx tests with UBSan. This demonstrates that your patch
is clearly insufficient. Further, Vladimir's patch is clearly
insufficient too, as shown for the another patch in the same
patch series.
If we want to follow this approach, we need some way to trace
places where zero length memcpy() can occur. My best guess is
that the only way is to look through all ngx_cpymem() /
ngx_memcpy() / ngx_memmove() / ngx_movemem() calls, as nginx
routinely uses { 0, NULL } as an empty string. Given that there
are 600+ of such calls in the codebase, and at least some need
serious audit to find out if { 0, NULL } can appear in the call,
this is going to be a huge work. And, given that the only
expected effect is theoretical correctness of the code, I doubt it
worth the effort, especially given that the end result will likely
reduce readability of the code.
--
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
> This demonstrates that your patch > is clearly insufficient. Further, Vladimir's patch is clearly > insufficient too, as shown for the another patch in the same > patch series. "Insufficient" only when compared to a hypothetical perfectly exhaustive patch that requires "huge work," as you put it. It's best not to let the perfect be the enemy of the good. Avoiding UB in normal program execution (as opposed to the test suite) will prevent common workloads from executing UB, which is not merely an issue of "theoretical correctness." See https://blog.regehr.org/archives/213 (section "A Fun Case Analysis") for an example of how this "NULL used in nonnull context" issue leads to unexpected program behavior. Thus, I think the best approach is to patch pstrdup to avoid memcpy-from-NULL, and patch other functions only if someone can present a backtrace from a real configuration of nginx that executed UB. -Ben ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello! On Tue, Jan 09, 2024 at 04:18:06PM +, Ben Kallus wrote: > > This demonstrates that your patch > > is clearly insufficient. Further, Vladimir's patch is clearly > > insufficient too, as shown for the another patch in the same > > patch series. > > "Insufficient" only when compared to a hypothetical perfectly exhaustive > patch that requires "huge work," as you put it. It's best not to let the > perfect be the enemy of the good. > > Avoiding UB in normal program execution (as opposed to the test suite) will > prevent common workloads from executing UB, which is not merely an issue of > "theoretical correctness." See https://blog.regehr.org/archives/213 > (section "A Fun Case Analysis") for an example of how this "NULL used in > nonnull context" issue leads to unexpected program behavior. > > Thus, I think the best approach is to patch pstrdup to avoid > memcpy-from-NULL, and patch other functions only if someone can present a > backtrace from a real configuration of nginx that executed UB. Thank you for your opinion. As I tried to explain in the review of Vladimir's patches, fixing scattered sanitizer reports individually, assuming no direct impact is present, has an obvious downside: as long as there is a consistent coding pattern which causes such reports, fixing individual reports will hide the pattern from being seen by the sanitizer, but won't eliminate it. As such, it will greatly reduce pressure on fixing the pattern, but if the pattern is indeed practically dangerous and has security consequences, it will be trivial for an attacker to find out cases which are not fixed and exploit them. As such, I prefer to identify patterns and fix them consistently over the code base instead of trying to quench individual reports. Quenching individual reports makes sense if we don't want to fix the pattern, assuming it is completely harmless anyway, but rather want to simplify usage of the sanitizer to identify other issues. This does not look like what you are advocating about though. (Also, again, patching just ngx_pstrdup() is clearly not enough even for this, see Vladimir's patch for a list of other places reported by UBSan in perfectly real configurations.) As already pointed out previously, there are no known cases when memcpy(p, NULL, 0) can result in miscompilation of nginx code, as nginx usually does not checks string data pointers against NULL (and instead checks length, if needed). In particular, ngx_pstrdup() you are trying to patch doesn't. That is, this is exactly the "no direct impact" situation as assumed above. If you think there are cases when the code can be miscompiled in practice, and not theoretically, please share. -- Maxim Dounin http://mdounin.ru/ ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hi Maxim, > As already pointed out previously, there are no known cases > when memcpy(p, NULL, 0) can result in miscompilation of nginx > code, ... If you think there are cases when the code can be > miscompiled in practice, and not theoretically, please share. There is no such thing as "miscompilation" of code that executes undefined behavior. The behavior is undefined; literally any instructions that the compiler emits is correct compilation. This is the definition of undefined behavior. You want me to cite a line in nginx that you would consider "miscompiled in practice." I'm not going to spend hours combing through assembly to convince you that undefined behavior is worth avoiding. Sorry! > as nginx usually does not checks string data pointers > against NULL (and instead checks length, if needed). In > particular, ngx_pstrdup() you are trying to patch doesn't. That > is, this is exactly the "no direct impact" situation as assumed > above. It is non-obvious that checks against NULL will be optimized away after calls to ngx_memcpy. Whether a function even calls ngx_memcpy on a given pointer may not always be obvious, especially if that call happens many layers down the stack. The argument "but we just don't do checks against NULL after calling memcpy" (much like the argument "but we just don't pass macro arguments that would violate operator precedence or cause side effects to be reevaluated") is a bad one because it requires all contributors to follow a set of undocumented rules which, when broken, can have serious consequences. Futher, the rules will change in the future as new compiler optimizations are developed and enabled by default. For now, NULL checks are to be avoided after calling memcpy on GCC -O2. In the future, it's plausible that they may need to be avoided before calling memcpy as well, and on clang -O1. Compiler authors do not all care about the fact that you believe that the standard is wrong. Their job is to make compliant programs run correctly and fast; they have no similar obligation for programs that execute UB. Most other C projects accept that avoiding all UB is necessary, even when somewhat inconvenient. Search "memcpy null" in the commit history of your C or C++ project of choice and you can see this for yourself. Every project I tried (OpenSSL, Apache httpd, FFmpeg, curl, WebKit, CPython) has at least 2 commits that work around memcpy from NULL, usually by adding a length check. As for what is the nicest way to avoid NULL memcpy, that is a matter of taste. I personally think that it is needless to add an extra branch to every memcpy, even when that memcpy's arguments are known to be nonnull. I therefore advocate for a piecemeal approach, which also seems to be the more common one. Patching ngx_memcpy, as you suggest, is also a valid solution to the issue, and which is better is a matter of opinion on which I don't have strong feelings. -Ben ( There is a proposal for C2y to define memcpy(NULL,NULL,0): https://docs.google.com/document/d/1guH_HgibKrX7t9JfKGfWX2UCPyZOTLsnRfR6UleD1F8/edit If you feel strongly that memcpy from NULL should be defined, feel free to contribute to it :) ) ___ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: Core: Avoid memcpy from NULL
Hello! On Wed, Jan 24, 2024 at 12:09:02AM +, Ben Kallus wrote: > > As already pointed out previously, there are no known cases > > when memcpy(p, NULL, 0) can result in miscompilation of nginx > > code, ... If you think there are cases when the code can be > > miscompiled in practice, and not theoretically, please share. > > There is no such thing as "miscompilation" of code that executes > undefined behavior. The behavior is undefined; literally any > instructions that the compiler emits is correct compilation. This is > the definition of undefined behavior. While it is certainly true, this is purely theoretical. In practice, real things happen: the code is compiled even to something that is either correct or not. In all known cases so far the compiled code is correct even if GCC with relevant (mis)optimizations enabled is used. > You want me to cite a line in nginx that you would consider > "miscompiled in practice." I'm not going to spend hours combing > through assembly to convince you that undefined behavior is worth > avoiding. Sorry! There is no need to convince me that undefined behaviour worth avoiding. The point is that patching it in the particular place you are trying to patch will make things worse by reducing pressure on developers, not better. And there is no immediate need to patch this particular place. Instead, we should ensure that a safe coding pattern is used across the code - either by patching ngx_memcpy() and other functions to check for length 0, or by reviewing and fixing all the affected calls we are able to find, or by explicitly asking GCC to avoid such misoptimizations (such as with -fno-delete-null-pointer-check like Linux kernel does), or by fixing the standard. [...] > > as nginx usually does not checks string data pointers > > against NULL (and instead checks length, if needed). In > > particular, ngx_pstrdup() you are trying to patch doesn't. That > > is, this is exactly the "no direct impact" situation as assumed > > above. > > It is non-obvious that checks against NULL will be optimized away > after calls to ngx_memcpy. Whether a function even calls ngx_memcpy on > a given pointer may not always be obvious, especially if that call > happens many layers down the stack. In the particular place you are trying to patch, it is quite obvious, even assuming link-time optimizations, since ngx_pstrdup() is called in very few places. And this can be easily verified at least for a particular compiler and compiler options. For other places, that's indeed might not be obvious (but see below), and that's why I asking you to share cases of the nginx code miscompiled if you know any. The point is, however, that the change you suggests with patching just ngx_pstrdup() won't fix these other places. Instead, it will rather ensure these other places are never fixed. OTOH, gcc13 with -O2 removes very few NULL pointer checks across nginx codebase. In my tests (with "-S" added to compilation to obtain assembler output), -fno-delete-null-pointer-check affects only 31 files, and files I checked only have few pointer checks removed (restored with -fno-delete-null-pointer-check): $ diff -urN objs.o2/src/ objs.o2fnodelete/src/ | diffstat core/ngx_inet.o | 703 +-- core/ngx_log.o| 555 +-- core/ngx_open_file_cache.o| 581 +-- core/ngx_output_chain.o | 771 ++-- core/ngx_palloc.o | 153 core/ngx_radix_tree.o | 327 - core/ngx_resolver.o | 2252 ++-- event/ngx_event.o | 850 ++-- event/ngx_event_openssl.o | 3576 ++- event/ngx_event_openssl_stapling.o|6 event/ngx_event_timer.o | 18 event/ngx_event_udp.o | 127 http/modules/ngx_http_auth_basic_module.o | 12 http/modules/ngx_http_charset_filter_module.o |7 http/modules/ngx_http_fastcgi_module.o| 2134 +-- http/modules/ngx_http_geo_module.o| 136 http/modules/ngx_http_image_filter_module.o | 186 - http/modules/ngx_http_limit_conn_module.o | 103 http/modules/ngx_http_log_module.o| 948 ++--- http/modules/ngx_http_secure_link_module.o| 34 http/modules/ngx_http_slice_filter_module.o | 77 http/modules/ngx_http_sub_filter_module.o | 126 http/ngx_http_file_cache.o| 118 http/ngx_http_parse.o | 842 ++-- http/ngx_http_script.o| 16 http/ngx_http_upstream.o | 4673 +- stream/ngx_stream_geo_module.o| 134 stream/ngx_stream_limit_conn_module.o | 105 stream/ngx_stream_log_module.o| 872 ++-- stream/ngx_stream_proxy_module.o
Re: Core: Avoid memcpy from NULL
Hi, Here's a similar ticket in another OSS. https://github.com/bellard/quickjs/issues/225#issuecomment-1908279228 > QuickJS may pass NULL pointers to memcpy with zero size. The C spec tells it is an undefined behavior but most C code do it, so the spec should be fixed instead. On Wed, Jan 24, 2024 at 4:57 PM Maxim Dounin wrote: > Hello! > > On Wed, Jan 24, 2024 at 12:09:02AM +, Ben Kallus wrote: > > > > As already pointed out previously, there are no known cases > > > when memcpy(p, NULL, 0) can result in miscompilation of nginx > > > code, ... If you think there are cases when the code can be > > > miscompiled in practice, and not theoretically, please share. > > > > There is no such thing as "miscompilation" of code that executes > > undefined behavior. The behavior is undefined; literally any > > instructions that the compiler emits is correct compilation. This is > > the definition of undefined behavior. > > While it is certainly true, this is purely theoretical. In > practice, real things happen: the code is compiled even to > something that is either correct or not. In all known cases so > far the compiled code is correct even if GCC with relevant > (mis)optimizations enabled is used. > > > You want me to cite a line in nginx that you would consider > > "miscompiled in practice." I'm not going to spend hours combing > > through assembly to convince you that undefined behavior is worth > > avoiding. Sorry! > > There is no need to convince me that undefined behaviour worth > avoiding. The point is that patching it in the particular place > you are trying to patch will make things worse by reducing > pressure on developers, not better. And there is no immediate > need to patch this particular place. > > Instead, we should ensure that a safe coding pattern is used > across the code - either by patching ngx_memcpy() and other > functions to check for length 0, or by reviewing and fixing all > the affected calls we are able to find, or by explicitly asking > GCC to avoid such misoptimizations (such as with > -fno-delete-null-pointer-check like Linux kernel does), or by > fixing the standard. > > [...] > > > > as nginx usually does not checks string data pointers > > > against NULL (and instead checks length, if needed). In > > > particular, ngx_pstrdup() you are trying to patch doesn't. That > > > is, this is exactly the "no direct impact" situation as assumed > > > above. > > > > It is non-obvious that checks against NULL will be optimized away > > after calls to ngx_memcpy. Whether a function even calls ngx_memcpy on > > a given pointer may not always be obvious, especially if that call > > happens many layers down the stack. > > In the particular place you are trying to patch, it is quite > obvious, even assuming link-time optimizations, since > ngx_pstrdup() is called in very few places. And this can be > easily verified at least for a particular compiler and compiler > options. > > For other places, that's indeed might not be obvious (but see > below), and that's why I asking you to share cases of the nginx > code miscompiled if you know any. The point is, however, that the > change you suggests with patching just ngx_pstrdup() won't fix > these other places. Instead, it will rather ensure these other > places are never fixed. > > OTOH, gcc13 with -O2 removes very few NULL pointer checks across > nginx codebase. In my tests (with "-S" added to compilation to > obtain assembler output), -fno-delete-null-pointer-check affects > only 31 files, and files I checked only have few pointer checks > removed (restored with -fno-delete-null-pointer-check): > > $ diff -urN objs.o2/src/ objs.o2fnodelete/src/ | diffstat > core/ngx_inet.o | 703 +-- > core/ngx_log.o| 555 +-- > core/ngx_open_file_cache.o| 581 +-- > core/ngx_output_chain.o | 771 ++-- > core/ngx_palloc.o | 153 > core/ngx_radix_tree.o | 327 - > core/ngx_resolver.o | 2252 ++-- > event/ngx_event.o | 850 ++-- > event/ngx_event_openssl.o | 3576 ++- > event/ngx_event_openssl_stapling.o|6 > event/ngx_event_timer.o | 18 > event/ngx_event_udp.o | 127 > http/modules/ngx_http_auth_basic_module.o | 12 > http/modules/ngx_http_charset_filter_module.o |7 > http/modules/ngx_http_fastcgi_module.o| 2134 +-- > http/modules/ngx_http_geo_module.o| 136 > http/modules/ngx_http_image_filter_module.o | 186 - > http/modules/ngx_http_limit_conn_module.o | 103 > http/modules/ngx_http_log_module.o| 948 ++--- > http/modules/ngx_http_secure_link_module.o| 34 > http/modules/ngx_http_slice_filter_module.o | 77 > http/modules/ngx_http_sub_filter_module.o
