Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)

Em sex., 12 de fev. de 2021 às 03:56, Kyotaro Horiguchi <
horikyota(dot)ntt(at)gmail(dot)com> escreveu:

> At Wed, 10 Feb 2021 20:12:38 -0300, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
> wrote in
> > Hi,
> >
> > Per Coverity.
> >
> > If xid is a subtransaction, the setup of base snapshot on the top-level
> > transaction,
> > can be not optional, otherwise a Dereference null return value
> > (NULL_RETURNS) can be raised.
> >
> > Patch suggestion to fix this.
> >
> > diff --git a/src/backend/replication/logical/reorderbuffer.c
> > b/src/backend/replication/logical/reorderbuffer.c
> > index 5a62ab8bbc..3c6a81f716 100644
> > --- a/src/backend/replication/logical/reorderbuffer.c
> > +++ b/src/backend/replication/logical/reorderbuffer.c
> > @@ -2993,8 +2993,8 @@ ReorderBufferSetBaseSnapshot(ReorderBuffer *rb,
> > TransactionId xid,
> > */
> > txn = ReorderBufferTXNByXid(rb, xid, true, &is_new, lsn, true);
> > if (rbtxn_is_known_subxact(txn))
> > - txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, false,
> > - NULL, InvalidXLogRecPtr, false);
> > + txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, true,
> > + NULL, InvalidXLogRecPtr, true);
> > Assert(txn->base_snapshot == NULL);
>
> If the return from the first call is a subtransaction, the second call
> always obtain the top transaction. If the top transaction actualy did
> not exist, it's rather the correct behavior to cause SEGV, than
> creating a bogus rbtxn. THus it is wrong to set create=true and
> create_as_top=true. We could change the assertion like Assert (txn &&
> txn->base_snapshot) to make things clearer.
>
It does not make sense to generate a SEGV on purpose and
assertion would not solve why this happens in a production environment.
Better to report an error if the second call fails.
What do you suggest as a description of the error?

regards,
Ranier Vilela