Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

On Thu, Sep 03, 2020 at 10:53:37PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > We do assume dereferencing NULL would crash, but we also assume this
> > optimization doesn't happen:
> 
> > #ifndef REMOVE_MEMCPY
> >     memcpy(dest, src, n);
> > #endif
> >     if (src)
> >     pause();
> 
> > [ gcc believes the if-test is unnecessary ]
> 
> > So yes, it would be reasonable to adopt -fno-delete-null-pointer-checks and/or
> > remove -fno-sanitize=nonnull-attribute from buildfarm member thorntail.

> If there actually are places where this is a problem, I think we
> need to fix it by doing
> 
>     if (n > 0)
>         memcpy(dest, src, n);
> 
> so that the compiler can no longer assume that src!=NULL even
> when n is zero.  I'd still leave -fdelete-null-pointer-checks
> enabled, because it can make valid and useful optimizations in
> other cases.  (Besides that, it's far from clear that disabling
> that flag would suppress all bad consequences of the assumption
> that memcpy's arguments aren't null.)

Your proposal is what I had in mind when I wrote "remove
-fno-sanitize=nonnull-attribute from buildfarm member thorntail", and I agree
it's attractive.  In particular, gcc is not likely to be the last compiler to
attempt such an optimization, and other compilers may not offer
-fno-delete-null-pointer-checks or equivalent.

One might argue for -fno-delete-null-pointer-checks in addition, because it
would defend against cases that sanitizers miss.  I tend to think that's
overkill, but maybe not.  I suppose one could do an audit, diffing the
generated code with and without the option.