Core: Avoid memcpy from NULL
洪志道
hongzhidao at gmail.com
Thu Jan 25 03:53:00 UTC 2024
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 <mdounin at mdounin.ru> wrote:
> 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/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20240125/19659f5c/attachment-0001.htm>
More information about the nginx-devel
mailing list