Opened 8 months ago

Closed 6 months ago

#2570 closed defect (wontfix)

memcpy from NULL during startup

Reported by: kenballus@… Owned by:
Priority: minor Milestone:
Component: nginx-core Version:
Keywords: Cc:
uname -a: Linux bfd92496df4c 6.1.63-1-lts #1 SMP PREEMPT_DYNAMIC Mon, 20 Nov 2023 12:45:57 +0000 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.25.4
built by gcc 12.2.0 (Debian 12.2.0-14)
configure arguments:

Description

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:

  1. Build and install Nginx with GCC 12.2.0 on x86_64 Linux using the default configuration.
  2. 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
    
  3. 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.

Change History (7)

comment:1 by kenballus@…, 8 months ago

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

comment:2 by kenballus@…, 8 months ago

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.

comment:3 by Maxim Dounin, 8 months ago

Component: documentationnginx-core
Version: 1.25.x

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.

comment:4 by kenballus@…, 8 months ago

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

in reply to:  4 comment:5 by Maxim Dounin, 8 months ago

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.

comment:6 by kenballus@…, 8 months ago

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.

comment:7 by Maxim Dounin, 6 months ago

Resolution: wontfix
Status: newclosed

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.

Note: See TracTickets for help on using tickets.