Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c) - Mailing list pgsql-hackers

From Ranier Vilela
Subject Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
Date
Msg-id CAEudQAoOkJs_X=E=12QLG-ys_+cmgDQrc2q=bsCjg-Q3ARviww@mail.gmail.com
Whole thread Raw
In response to Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)  (Kyotaro Horiguchi <horikyota.ntt@gmail.com>)
List pgsql-hackers
Em sex., 12 de fev. de 2021 às 03:56, Kyotaro Horiguchi <horikyota.ntt@gmail.com> escreveu:
At Wed, 10 Feb 2021 20:12:38 -0300, Ranier Vilela <ranier.vf@gmail.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

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: Keep notnullattrs in RelOptInfo (Was part of UniqueKey patch series)
Next
From: Zhihong Yu
Date:
Subject: Re: Why do we have MakeSingleTupleTableSlot instead of not using MakeTupleTableSlot?