Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

On Thu, Sep 03, 2020 at 10:53:37PM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)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.