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