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

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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

On Fri, Feb 12, 2021 at 03:56:02PM +0900, Kyotaro Horiguchi wrote:

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.

I don't see much the point to change this code. The result would be
the same: a PANIC at this location.
--
Michael

Hi,
How about the following patch ?

ReorderBufferSetBaseSnapshot() can return a bool to indicate whether the
base snapshot is set up.

For the call by SnapBuildCommitTxn(), it seems xid is top transaction. So
the return value doesn't need to be checked.

Cheers

On Fri, Feb 12, 2021 at 6:40 PM Michael Paquier <michael@paquier.xyz> wrote:

Em sáb., 13 de fev. de 2021 às 01:07, Zhihong Yu <zyu@yugabyte.com>
escreveu:

Hi,
How about the following patch ?

ReorderBufferSetBaseSnapshot() can return a bool to indicate whether the
base snapshot is set up.

For the call by SnapBuildCommitTxn(), it seems xid is top transaction. So
the return value doesn't need to be checked.

IMO anything else is better than PANIC.
Anyway, if all fails, reporting an error can contribute to checking where.

Attached a patch suggestion v2.
1. SnapBuildProcessChange returns a result of ReorderBufferSetBaseSnapshot,
so the caller can act accordingly.
2. SnapBuildCommitTxn can't ignore a result
from ReorderBufferSetBaseSnapshot, even if it never fails.

regards,
Ranier Vilela

Em sáb., 13 de fev. de 2021 às 17:35, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

Em sáb., 13 de fev. de 2021 às 01:07, Zhihong Yu <zyu@yugabyte.com>
escreveu:

Hi,
How about the following patch ?

ReorderBufferSetBaseSnapshot() can return a bool to indicate whether the
base snapshot is set up.

For the call by SnapBuildCommitTxn(), it seems xid is top transaction. So
the return value doesn't need to be checked.

IMO anything else is better than PANIC.
Anyway, if all fails, reporting an error can contribute to checking where.

Attached a patch suggestion v2.

Sorry, I forgot to mention, it is based on a patch from Zhihong Yu.

regards,
Ranier Vilela

Hi,
+               (errmsg("BaseSnapshot cant't be setup at point %X/%X",
+                       (uint32) (lsn >> 32), (uint32) lsn),
+                errdetail("Top transaction is running.")));

Did you mean this errdetail:

Top transaction is not running.

Cheers

On Sat, Feb 13, 2021 at 12:34 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

Em sáb., 13 de fev. de 2021 às 17:48, Zhihong Yu <zyu@yugabyte.com>
escreveu:

Hi,
+               (errmsg("BaseSnapshot cant't be setup at point %X/%X",
+                       (uint32) (lsn >> 32), (uint32) lsn),
+                errdetail("Top transaction is running.")));

Did you mean this errdetail:

Top transaction is not running.

Done.

Thanks Zhihong.
v3 based on your patch, attached.

regards,
Ranier Vilela

Hi,
Patch v4 corrects a small typo:
+ (errmsg("BaseSnapshot cant't be setup at point %X/%X",

Cheers

On Sat, Feb 13, 2021 at 12:58 PM Ranier Vilela <ranier.vf@gmail.com> wrote: