Core: Avoid memcpy from NULL

Wed Jan 24 08:57:16 UTC 2024

Hello!

On Wed, Jan 24, 2024 at 12:09:02AM +0000, 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              |  749 ++--
 stream/ngx_stream_script.o                    |   16 
 31 files changed, 10658 insertions(+), 10549 deletions(-)

In particular, in the only real change in ngx_palloc.o assembler 
is as follows:

@@ -452,16 +452,19 @@ ngx_reset_pool:
        testl   %ebx, %ebx
        jne     .L99 .L97:
+       testl   %esi, %esi
+       je      .L100
        movl    %esi, %eax
        .p2align 4,,10
        .p2align 3

Which corresponds to restored with -fno-delete-null-pointer-check 
initial "p" check on the first iteration of the second loop, due 
to "pool->large" being used in the first loop initialization:

    for (l = pool->large; l; l = l->next) {
        if (l->alloc) {
            ngx_free(l->alloc);
        }
    }

    for (p = pool; p; p = p->d.next) {
        p->d.last = (u_char *) p + sizeof(ngx_pool_t);
        p->d.failed = 0;
    }

Many other cases, such as multiple changes in ngx_inet.o, one of 
the two changes in ngx_open_file_cache.o, and one change 
ngx_http_parse.o, correspond to ngx_strlchr() calls, where 
subsequent result check is omitted because no-match is directly 
handled by the inlined ngx_strlchr() code:

    p = ngx_strlchr(uri->data, last, '?');

    if (p) { ... }

And there seems to be no real changes in ngx_http_script.o and 
ngx_stream_script.o, just some minor instructions reordering.

Overall, it might be feasible to review all the differences to 
ensure there are no real issues introduced by various compiler 
optimizations.

[...]

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

Interesting, thanks.  This would be the best solution and will 
eliminate the need for any changes by defining the behaviour, as 
it should.

-- 
Maxim Dounin
http://mdounin.ru/
