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