Core: Avoid memcpy from NULL

[Maxim Dounin]

Hello!

On Fri, Dec 15, 2023 at 03:46:19PM +0100, Dipl. Ing. Sergey Brester via nginx-devel wrote:

> Enclosed few thoughts to the subject:
> 
> - since it is very rare situation that one needs only a memcpy without 
> to know whether previous alloc may fail
>    (e. g. some of pointers were NULL), me too thinks that the caller 
> should be responsible for the check.
>    So I would not extend ngx_memcpy or ngx_cpymem in that way.

That's not about failed allocations, it's about ability to work 
with empty strings which aren't explicitly set to something other 
than { 0, NULL }.  And you may refer to Vladimir's patch I've 
referenced to find out what it means in terms of the "caller 
should be responsible" approach even without trying to look into 
places not explicitly reported by the UB sanitizer.

https://mailman.nginx.org/pipermail/nginx-devel/2023-October/PX7VH5A273NLUGSYC7DR2AZRU75CIQ3Q.html
https://mailman.nginx.org/pipermail/nginx-devel/2023-December/DCGUEGEFS6TSVIWNEWUEZO3FZMR6ESYZ.html

> - rewrite of `ngx_memcpy` define like here:
>    ```
>    + #define ngx_memcpy(dst, src, n) (void) ((n) == 0 ? (dst) : 
> memcpy(dst, src, n))
>    ```
>    may introduce a regression or compat issues, e. g. fully functioning 
> codes like that may become broken hereafter:
>    ```
>    ngx_memcpy(dst, src, ++len); // because n would be incremented twice 
> in the macro now
>    ```
>    Sure, `ngx_cpymem` has also the same issue, but it had that already 
> before the "fix"...
>    Anyway, I'm always against of such macros (no matter with or without 
> check it would be better as an inline function instead).

In general macro definitions in nginx are used everywhere for 
efficiency reasons, and macro definitions usually aren't safe.  
While some might prefer other approaches, writing code like 
"ngx_memcpy(dst, src, ++len)" in nginx is just wrong, and 
shouldn't be trusted to work, much like it won't work with 
"ngx_cpymem(dst, src, ++len)".

I'm not exactly against using inline functions here, but the 
particular argument is at most very weak. 

> My conclusion:
>    a fix of affected places invoking `ngx_memcpy` and `ngx_cpymem`, and 
> possibly an assert in `ngx_memcpy`
>    and `ngx_cpymem` would be fully enough, in my opinion.

Well, thank you for your opinion, appreciated.  I don't think this 
approach is going to work though, see my review of Vladimir's 
patch.

Ideally, I would prefer this to be fixed in the C standard (and 
GCC).  But given this is not a likely option, and there is a 
constant stream of reports "hey, UB sanitizer reports about 
memcpy(dst, NULL, 0) in nginx" we might consider actually 
silencing this by introducing appropriate checks at the interface 
level.

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