Re: Clang UndefinedBehaviorSanitize (Postgres14) Detected undefined-behavior

Em sex., 28 de ago. de 2020 às 03:04, Noah Misch <noah(at)leadboat(dot)com>
escreveu:

> On Thu, Aug 27, 2020 at 11:11:47PM -0400, Tom Lane wrote:
> > Noah Misch <noah(at)leadboat(dot)com> writes:
> > > On Thu, Aug 27, 2020 at 12:57:20PM -0400, Alvaro Herrera wrote:
> > >> Looks legit, and at least per commit 13bba02271dc we do fix such
> things,
> > >> even if it's useless in practice.
> > >> Given that no buildfarm member has ever complained, this exercise
> seems
> > >> pretty pointless.
> >
> > > Later decision to stop changing such code:
> > >
> https://postgr.es/m/flat/e1a26ece-7057-a234-d87e-4ce1cdc9eaa0(at)2ndquadrant(dot)com
>
> > I think that the actual risk here has to do not with
> > what memcmp() might do, but with what gcc might do in code surrounding
> > the call, once it's armed with the assumption that any pointer we pass
> > to memcmp() could not be null. See
> >
> >
> https://www.postgresql.org/message-id/flat/BLU437-SMTP48A5B2099E7134AC6BE7C7F2A10%40phx.gbl
> >
> > It's surely not hard to visualize cases where necessary code could
> > be optimized away if the compiler thinks it's entitled to assume
> > such things.
>
> Good point. We could pick from a few levels of concern:
>
> - No concern: reject changes serving only to remove this class of
> deviation.
> This is today's policy.
> - Medium concern: accept fixes, but the buildfarm continues not to break in
> the face of new deviations. This will make some code uglier, but we'll
> be
> ready against some compiler growing the optimization you describe.
> - High concern: I remove -fno-sanitize=nonnull-attribute from buildfarm
> member
> thorntail. In addition to the drawback of the previous level, this will
> create urgent work for committers introducing new deviations (or
> introducing
> test coverage that unearths old deviations). This is our current
> response
> to Valgrind complaints, for example.
>
Maybe in this specific case, the policy could be ignored, this change does
not hurt.

--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -293,7 +293,7 @@ TransactionIdSetPageStatus(TransactionId xid, int
nsubxids,
* sub-XIDs and all of the XIDs for which we're adjusting clog should be
* on the same page. Check those conditions, too.
*/
- if (all_xact_same_page && xid == MyProc->xid &&
+ if (all_xact_same_page && subxids && xid == MyProc->xid &&
nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT &&
nsubxids == MyProc->subxidStatus.count &&
memcmp(subxids, MyProc->subxids.xids,

regards,
Ranier Vilela