memcpy from NULL during startup

[kenballus@…]

memcpying to or from NULL is undefined behavior, even when the copy is of size 0.

Nginx executes a memcpy from NULL during startup with the default configuration. This can be reproduced very easily as follows:

0. Build and install Nginx with GCC 12.2.0 on x86_64 Linux using the default configuration.
1. Run Nginx under gdb, setting the appropriate breakpoints:

   gdb -ex 'b ngx_pstrdup' -ex 'r' -ex 'c' -ex 'c' -ex 'b memcpy' -ex 'c' -ex 'print $rsi' /usr/local/nginx/sbin/nginx
   

2. Observe that the second argument to memcpy is NULL:

   GNU gdb (Debian 13.1-3) 13.1
   Copyright (C) 2023 Free Software Foundation, Inc.
   License GPLv3+: GNU GPL version 3 or later <LINK OMITTED>
   This is free software: you are free to change and redistribute it.
   There is NO WARRANTY, to the extent permitted by law.
   Type "show copying" and "show warranty" for details.
   This GDB was configured as "x86_64-linux-gnu".
   Type "show configuration" for configuration details.
   For bug reporting instructions, please see:
   <LINK OMITTED>.
   Find the GDB manual and other documentation resources online at:
       <LINK OMITTED>.
   
   For help, type "help".
   Type "apropos word" to search for commands related to "word"...
   Reading symbols from /usr/local/nginx/sbin/nginx...
   Breakpoint 1 at 0x1ea6c: file src/core/ngx_string.c, line 76.
   Starting program: /usr/local/nginx/sbin/nginx
   warning: Error disabling address space randomization: Operation not permitted
   [Thread debugging using libthread_db enabled]
   Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
   
   Breakpoint 1, ngx_pstrdup (pool=pool@entry=0x55c897c5e4d0, src=src@entry=0x7ffcbb97b358)
       at src/core/ngx_string.c:76
   76	{
   Continuing.
   
   Breakpoint 1, ngx_pstrdup (pool=pool@entry=0x55c897c5e4d0, src=src@entry=0x7ffcbb97b368)
       at src/core/ngx_string.c:76
   76	{
   Continuing.
   
   Breakpoint 1, ngx_pstrdup (pool=pool@entry=0x55c897c5e4d0, src=src@entry=0x7ffcbb97b348)
       at src/core/ngx_string.c:76
   76	{
   Breakpoint 2 at 0x7f3413660cc0: memcpy. (4 locations)
   Continuing.
   
   Breakpoint 2.3, __memcpy_avx_unaligned_erms ()
       at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:264
   264	../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S: No such file or directory.
   $1 = 0
   

This was found with UBSan. I have tested it both on the development branch, as well as 1.22.1, and the issue is present in both.

[kenballus@…]

Here's a backtrace:

#0  __memcpy_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:264
#1  0x000056019d9df5e6 in ngx_pstrdup (pool=pool@entry=0x56019efea4d0, src=src@entry=0x7ffe44678bc8)
    at src/core/ngx_string.c:84
#2  0x000056019d9ec0ee in ngx_init_cycle (old_cycle=old_cycle@entry=0x7ffe446789a0)
    at src/core/ngx_cycle.c:118
#3  0x000056019d9d9e4a in main (argc=1, argv=0x7ffe44678d68) at src/core/nginx.c:293

[kenballus@…]

Now noticing that this has component "documentation." That was a mistake; this should be in "core." If anyone with permission to change the component to "core" would do so, I would appreciate it.

[Maxim Dounin]

While memcpy(p, NULL, 0) is formally undefined behaviour due to C standard wording (see ​here), the expected behaviour for memcpy(p, NULL, 0) is perfectly obvious, and any compiler which miscompiles such code is utterly broken. And yes, recent gcc versions with -O2 (or -O1 -ftree-vrp) are broken, as they used to remove NULL pointer checks on pointers passed to memcpy(), assuming they are not NULL.

Still, we haven't seen any issues with miscompiled code in nginx. Specifically, ngx_pstrdup() mentioned in the report is always compiled properly, even with gcc -O2, as the function does not try to test the source pointer after memcpy(). Further, nginx uses -O1 by default, which is not affected at all.

Overall, while we are generally ok with quenching such complaints from various development tools, even dumb ones, this case is much broader than the particular ngx_pstrdup() call. A recent development thread about it can be found here:

​https://mailman.nginx.org/pipermail/nginx-devel/2023-October/PX7VH5A273NLUGSYC7DR2AZRU75CIQ3Q.html

I've just wrote a review for the patch about memcpy(p, NULL, 0):

​https://mailman.nginx.org/pipermail/nginx-devel/2023-December/DCGUEGEFS6TSVIWNEWUEZO3FZMR6ESYZ.html

If you think you known a good solution, feel free to participate.

[kenballus@…]

Even if we all agree that memcpy(*, *, 0) should always be valid, the standard says otherwise. Relying on UB is risky and difficult to get right.

I think this is worth changing in particular because it's difficult to reason about what compiler optimizations might exist in the future. For now, it's only an issue with gcc -O2 -fno-builtin, but what if this optimization gets moved from -02 to -01? What if clang starts doing this?

The following patch satisfies UBSan. I think it's a good balance between allowing 0-length memcpy and avoiding UB:

diff --git a/src/core/ngx_string.c b/src/core/ngx_string.c
index f8f738472..b52905c5a 100644
--- a/src/core/ngx_string.c
+++ b/src/core/ngx_string.c
@@ -77,9 +77,6 @@ ngx_pstrdup(ngx_pool_t *pool, ngx_str_t *src)
     u_char  *dst;

     dst = ngx_pnalloc(pool, src->len);
-    if (dst == NULL) {
-        return NULL;
-    }

     ngx_memcpy(dst, src->data, src->len);

@@ -2097,6 +2094,9 @@ ngx_memcpy(void *dst, const void *src, size_t n)
         ngx_log_error(NGX_LOG_ALERT, ngx_cycle->log, 0, "memcpy %uz bytes", n);
         ngx_debug_point();
     }
+    if (dst == NULL || n == 0) {
+        return dst;
+    }

     return memcpy(dst, src, n);
 }
diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
index 713eb42a7..e21b98216 100644
--- a/src/core/ngx_string.h
+++ b/src/core/ngx_string.h
@@ -103,8 +103,8 @@ void *ngx_memcpy(void *dst, const void *src, size_t n);
  * 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)   ( ((n) == 0 ? (u_char *) (dst) : (u_char *) memcpy(dst, src, n) + (n)))

 #endif

[Maxim Dounin]

Even if we all agree that memcpy(*, *, 0) should always be valid, the standard says otherwise. Relying on UB is risky and difficult to get right.

Sure, the best solution would be to fix the standard.

I think this is worth changing in particular because it's difficult to reason about what compiler optimizations might exist in the future. For now, it's only an issue with gcc -O2 -fno-builtin, but what if this optimization gets moved from -02 to -01? What if clang starts doing this?

The problem is that changing this would result in extra overhead for everybody, making code harder to read and penalizing compilers which actually produce valid code. And let me re-iterate: no cases of miscompilation of nginx code are actually seen, even with recent versions of gcc and -O2.

The following patch satisfies UBSan. I think it's a good balance between allowing 0-length memcpy and avoiding UB:

diff --git a/src/core/ngx_string.c b/src/core/ngx_string.c
index f8f738472..b52905c5a 100644
--- a/src/core/ngx_string.c
+++ b/src/core/ngx_string.c
@@ -77,9 +77,6 @@ ngx_pstrdup(ngx_pool_t *pool, ngx_str_t *src)
     u_char  *dst;

     dst = ngx_pnalloc(pool, src->len);
-    if (dst == NULL) {
-        return NULL;
-    }

     ngx_memcpy(dst, src->data, src->len);

This is simply wrong: you've removed allocation error handling for no reason, so with this change an allocation error will cause a segmentation fault due to NULL pointer dereference.

@@ -2097,6 +2094,9 @@ ngx_memcpy(void *dst, const void *src, size_t n)
         ngx_log_error(NGX_LOG_ALERT, ngx_cycle->log, 0, "memcpy %uz bytes", n);
         ngx_debug_point();
     }
+    if (dst == NULL || n == 0) {
+        return dst;
+    }

     return memcpy(dst, src, n);
 }

As long as dst is NULL and n isn't 0, the call should result in a NULL pointer dereference and a segfault, but with your change such bugs will be silently ignored.

diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
index 713eb42a7..e21b98216 100644
--- a/src/core/ngx_string.h
+++ b/src/core/ngx_string.h
@@ -103,8 +103,8 @@ void *ngx_memcpy(void *dst, const void *src, size_t n);
  * 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)   ( ((n) == 0 ? (u_char *) (dst) : (u_char *) memcpy(dst, src, n) + (n)))

 #endif

Yes, as suggested in the review referenced above, we could consider something like this. It needs some polishing though, as this probably could be written better, and certainly needs style fixes. Also, we probably should extend this to similarly handle ngx_memmove() / ngx_movemem() for consistency. And we might want to remove some explicit checks previously introduced (such as in 29795b697e14).

As previously suggested, don't hesitate to participate in the development thread I've referenced. Patch reviews are expected to happen in the development mailing list, not in Trac, see ​here.

[kenballus@…]

This is simply wrong: you've removed allocation error handling for no reason, so with this change an allocation error will cause a segmentation fault due to NULL pointer dereference.

As long as dst is NULL and n isn't 0, the call should result in a NULL pointer dereference and a segfault, but with your change such bugs will be silently ignored.

You're right; I misread my own diff and submitted a patch that was halfway between two changesets. Sorry.

Yes, as suggested in the review referenced above, we could consider something like this. It needs some polishing though, as this probably could be written better, and certainly needs style fixes.

Agreed. I'll move to the mailing list to address whether this is worth doing.

[Maxim Dounin]

As discussed in the mailing list (see ​here), there is a ​proposal to change the standard, so the behaviour will be defined as it should. Closing this.