Core: Avoid memcpy from NULL

[Maxim Dounin]

Hello!

On Tue, Jan 09, 2024 at 04:18:06PM +0000, Ben Kallus wrote:

> > This demonstrates that your patch
> > is clearly insufficient.  Further, Vladimir's patch is clearly
> > insufficient too, as shown for the another patch in the same
> > patch series.
> 
> "Insufficient" only when compared to a hypothetical perfectly exhaustive
> patch that requires "huge work," as you put it. It's best not to let the
> perfect be the enemy of the good.
> 
> Avoiding UB in normal program execution (as opposed to the test suite) will
> prevent common workloads from executing UB, which is not merely an issue of
> "theoretical correctness." See https://blog.regehr.org/archives/213
> (section "A Fun Case Analysis") for an example of how this "NULL used in
> nonnull context" issue leads to unexpected program behavior.
> 
> Thus, I think the best approach is to patch pstrdup to avoid
> memcpy-from-NULL, and patch other functions only if someone can present a
> backtrace from a real configuration of nginx that executed UB.

Thank you for your opinion.

As I tried to explain in the review of Vladimir's patches, fixing 
scattered sanitizer reports individually, assuming no direct 
impact is present, has an obvious downside: as long as there is a 
consistent coding pattern which causes such reports, fixing 
individual reports will hide the pattern from being seen by the 
sanitizer, but won't eliminate it.  As such, it will greatly 
reduce pressure on fixing the pattern, but if the pattern is 
indeed practically dangerous and has security consequences, it 
will be trivial for an attacker to find out cases which are not 
fixed and exploit them.  As such, I prefer to identify patterns 
and fix them consistently over the code base instead of trying to 
quench individual reports.

Quenching individual reports makes sense if we don't want to fix 
the pattern, assuming it is completely harmless anyway, but rather 
want to simplify usage of the sanitizer to identify other issues.  
This does not look like what you are advocating about though.  
(Also, again, patching just ngx_pstrdup() is clearly not enough 
even for this, see Vladimir's patch for a list of other places 
reported by UBSan in perfectly real configurations.)

As already pointed out previously, there are no known cases 
when memcpy(p, NULL, 0) can result in miscompilation of nginx 
code, 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.  If you think there are cases when the code can be 
miscompiled in practice, and not theoretically, please share.

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