<div dir="ltr">Hi,<div>Here's a similar ticket in another OSS.</div><div><a href="https://github.com/bellard/quickjs/issues/225#issuecomment-1908279228">https://github.com/bellard/quickjs/issues/225#issuecomment-1908279228</a><br></div><div><br></div><div>> <span style="color:rgb(31,35,40);font-family:-apple-system,"system-ui","Segoe UI","Noto Sans",Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji";font-size:14px">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.</span></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 24, 2024 at 4:57 PM Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello!<br>
<br>
On Wed, Jan 24, 2024 at 12:09:02AM +0000, Ben Kallus wrote:<br>
<br>
> > As already pointed out previously, there are no known cases<br>
> > when memcpy(p, NULL, 0) can result in miscompilation of nginx<br>
> > code, ... If you think there are cases when the code can be<br>
> > miscompiled in practice, and not theoretically, please share.<br>
> <br>
> There is no such thing as "miscompilation" of code that executes<br>
> undefined behavior. The behavior is undefined; literally any<br>
> instructions that the compiler emits is correct compilation. This is<br>
> the definition of undefined behavior.<br>
<br>
While it is certainly true, this is purely theoretical. In <br>
practice, real things happen: the code is compiled even to <br>
something that is either correct or not. In all known cases so <br>
far the compiled code is correct even if GCC with relevant <br>
(mis)optimizations enabled is used.<br>
<br>
> You want me to cite a line in nginx that you would consider<br>
> "miscompiled in practice." I'm not going to spend hours combing<br>
> through assembly to convince you that undefined behavior is worth<br>
> avoiding. Sorry!<br>
<br>
There is no need to convince me that undefined behaviour worth <br>
avoiding. The point is that patching it in the particular place <br>
you are trying to patch will make things worse by reducing <br>
pressure on developers, not better. And there is no immediate <br>
need to patch this particular place.<br>
<br>
Instead, we should ensure that a safe coding pattern is used <br>
across the code - either by patching ngx_memcpy() and other <br>
functions to check for length 0, or by reviewing and fixing all <br>
the affected calls we are able to find, or by explicitly asking <br>
GCC to avoid such misoptimizations (such as with <br>
-fno-delete-null-pointer-check like Linux kernel does), or by <br>
fixing the standard.<br>
<br>
[...]<br>
<br>
> > as nginx usually does not checks string data pointers<br>
> > against NULL (and instead checks length, if needed). In<br>
> > particular, ngx_pstrdup() you are trying to patch doesn't. That<br>
> > is, this is exactly the "no direct impact" situation as assumed<br>
> > above.<br>
> <br>
> It is non-obvious that checks against NULL will be optimized away<br>
> after calls to ngx_memcpy. Whether a function even calls ngx_memcpy on<br>
> a given pointer may not always be obvious, especially if that call<br>
> happens many layers down the stack.<br>
<br>
In the particular place you are trying to patch, it is quite <br>
obvious, even assuming link-time optimizations, since <br>
ngx_pstrdup() is called in very few places. And this can be <br>
easily verified at least for a particular compiler and compiler <br>
options.<br>
<br>
For other places, that's indeed might not be obvious (but see <br>
below), and that's why I asking you to share cases of the nginx <br>
code miscompiled if you know any. The point is, however, that the <br>
change you suggests with patching just ngx_pstrdup() won't fix <br>
these other places. Instead, it will rather ensure these other <br>
places are never fixed.<br>
<br>
OTOH, gcc13 with -O2 removes very few NULL pointer checks across <br>
nginx codebase. In my tests (with "-S" added to compilation to <br>
obtain assembler output), -fno-delete-null-pointer-check affects <br>
only 31 files, and files I checked only have few pointer checks <br>
removed (restored with -fno-delete-null-pointer-check):<br>
<br>
$ diff -urN objs.o2/src/ objs.o2fnodelete/src/ | diffstat<br>
core/ngx_inet.o | 703 +--<br>
core/ngx_log.o | 555 +--<br>
core/ngx_open_file_cache.o | 581 +--<br>
core/ngx_output_chain.o | 771 ++--<br>
core/ngx_palloc.o | 153 <br>
core/ngx_radix_tree.o | 327 -<br>
core/ngx_resolver.o | 2252 ++++++------<br>
event/ngx_event.o | 850 ++--<br>
event/ngx_event_openssl.o | 3576 ++++++++++---------<br>
event/ngx_event_openssl_stapling.o | 6 <br>
event/ngx_event_timer.o | 18 <br>
event/ngx_event_udp.o | 127 <br>
http/modules/ngx_http_auth_basic_module.o | 12 <br>
http/modules/ngx_http_charset_filter_module.o | 7 <br>
http/modules/ngx_http_fastcgi_module.o | 2134 +++++------<br>
http/modules/ngx_http_geo_module.o | 136 <br>
http/modules/ngx_http_image_filter_module.o | 186 -<br>
http/modules/ngx_http_limit_conn_module.o | 103 <br>
http/modules/ngx_http_log_module.o | 948 ++---<br>
http/modules/ngx_http_secure_link_module.o | 34 <br>
http/modules/ngx_http_slice_filter_module.o | 77 <br>
http/modules/ngx_http_sub_filter_module.o | 126 <br>
http/ngx_http_file_cache.o | 118 <br>
http/ngx_http_parse.o | 842 ++--<br>
http/ngx_http_script.o | 16 <br>
http/ngx_http_upstream.o | 4673 +++++++++++++-------------<br>
stream/ngx_stream_geo_module.o | 134 <br>
stream/ngx_stream_limit_conn_module.o | 105 <br>
stream/ngx_stream_log_module.o | 872 ++--<br>
stream/ngx_stream_proxy_module.o | 749 ++--<br>
stream/ngx_stream_script.o | 16 <br>
31 files changed, 10658 insertions(+), 10549 deletions(-)<br>
<br>
In particular, in the only real change in ngx_palloc.o assembler <br>
is as follows:<br>
<br>
@@ -452,16 +452,19 @@ ngx_reset_pool:<br>
testl %ebx, %ebx<br>
jne .L99 .L97:<br>
+ testl %esi, %esi<br>
+ je .L100<br>
movl %esi, %eax<br>
.p2align 4,,10<br>
.p2align 3<br>
<br>
Which corresponds to restored with -fno-delete-null-pointer-check <br>
initial "p" check on the first iteration of the second loop, due <br>
to "pool->large" being used in the first loop initialization:<br>
<br>
for (l = pool->large; l; l = l->next) {<br>
if (l->alloc) {<br>
ngx_free(l->alloc);<br>
}<br>
}<br>
<br>
for (p = pool; p; p = p->d.next) {<br>
p->d.last = (u_char *) p + sizeof(ngx_pool_t);<br>
p->d.failed = 0;<br>
}<br>
<br>
Many other cases, such as multiple changes in ngx_inet.o, one of <br>
the two changes in ngx_open_file_cache.o, and one change <br>
ngx_http_parse.o, correspond to ngx_strlchr() calls, where <br>
subsequent result check is omitted because no-match is directly <br>
handled by the inlined ngx_strlchr() code:<br>
<br>
p = ngx_strlchr(uri->data, last, '?');<br>
<br>
if (p) { ... }<br>
<br>
And there seems to be no real changes in ngx_http_script.o and <br>
ngx_stream_script.o, just some minor instructions reordering.<br>
<br>
Overall, it might be feasible to review all the differences to <br>
ensure there are no real issues introduced by various compiler <br>
optimizations.<br>
<br>
[...]<br>
<br>
> There is a proposal for C2y to define memcpy(NULL,NULL,0):<br>
> <a href="https://docs.google.com/document/d/1guH_HgibKrX7t9JfKGfWX2UCPyZOTLsnRfR6UleD1F8/edit" rel="noreferrer" target="_blank">https://docs.google.com/document/d/1guH_HgibKrX7t9JfKGfWX2UCPyZOTLsnRfR6UleD1F8/edit</a><br>
> If you feel strongly that memcpy from NULL should be defined, feel<br>
> free to contribute to it :)<br>
<br>
Interesting, thanks. This would be the best solution and will <br>
eliminate the need for any changes by defining the behaviour, as <br>
it should.<br>
<br>
-- <br>
Maxim Dounin<br>
<a href="http://mdounin.ru/" rel="noreferrer" target="_blank">http://mdounin.ru/</a><br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
<a href="https://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">https://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</blockquote></div>