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@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
