Core: Avoid memcpy from NULL

[Maxim Dounin]

Hello!

On Fri, Dec 29, 2023 at 04:50:36PM +0000, Ben Kallus wrote:

> > Still, -O0 is often used at least during development, and it might
> > be unreasonable to introduce extra function calls in basic
> > primitives.
> 
> I don't think this is a major cause for concern. It is perfectly
> reasonable for ngx_memcpy be a wrapper function around memcpy; I think
> most people would assume that from the name. In fact, it's *already*
> implemented as a function when NGX_MEMCPY_LIMIT is defined.

The NGX_MEMCPY_LIMIT is a very specific debugging define, which 
implies additional overhead for memcpy calls.  And it's certainly 
not an excuse for changing all the macro definitions to function 
calls.

Whether or not ngx_memcpy() can be implemented as a wrapper 
function is a separate question.

> > Further, nginx generally supports all available platforms
> > reasonably compatible with POSIX and C89.  This implies that
> > inline might be not available.
> 
> On such platforms, moving ngx_memcpy to a function may introduce some
> performance overhead. The question is whether slightly better
> performance on obscure systems is worth the mental overhead of working
> with function-like macros.

As said earlier in the thread, while some might prefer other 
approaches, function-like macros are used everywhere in nginx.

> > Sure (but see above about performance overhead; and another
> > question is if it needs to be solved, or following existing style
> > is enough to never see the issue).
> 
> A little extra performance overhead on C89 systems, or builds with -O0
> is a very small price to pay. ngx_resolver.c:4283 contains direct
> evidence that function-like macros incur mental overhead:
> ```
> ngx_memcpy(sin6->sin6_addr.s6_addr, addr6[j++].s6_addr, 16);
> ```
> Here we have an expression with a side-effect being passed into a
> function-like macro. As luck would have it, the second argument to
> ngx_memcpy is evaluated only once, so this is coincidentally okay.
> This particular landmine has been armed for a decade (see
> https://github.com/nginx/nginx/blob/769eded73267274e018f460dd76b417538aa5934/src/core/ngx_resolver.c#L2902).
> Thus, the existing style guidelines are not enough to prevent issues
> with function-like macros from arising in nginx. Inline functions
> solve this problem near-optimally.

The mentioned call specifically assumes that the second argument 
is only evaluated once, and it was certainly checked that it is 
only evaluated once when the call was written.  That is, there is 
no issue here.

Still, general style guidelines suggests that the code shouldn't 
be written this way, and the only reason for j++ in the line in 
question is that it mimics corresponding IPv4 code.

> > good luck with doing something like "ngx_max(foo & 0xff, bar)".
> 
> Nginx is littered with uses of expressions in ngx_max and ngx_min, it
> just happens that none of those expressions use operators of lower
> precedence than < and >. This seems like an invitation for human
> error.

It's not "just happens".

> Thus, I argue that the best solution to the memcpy-from-NULL problem
> is to replace ngx_memcpy and ngx_cpymem with inline functions with the
> appropriate checks for n==0. Going forward, it's probably smart to
> consider transitioning away from function-like macros more generally.

As I said earlier in this thread, I'm not exactly against using 
inline functions here, and I think a solution with inline 
functions can be accepted provided it is demonstrated that it 
introduces no measurable performance degradation.

Still, I'm very sceptical about the idea of "transitioning away 
from function-like macros".

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