Home > mailing lists

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

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

From

Ranier Vilela

Date:

11 February 2021, 02:12:38

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);

txn->base_snapshot = snap;

regards,

Ranier Vilela

Attachment

reorderbuffer.patch

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

From

Kyotaro Horiguchi

Date:

12 February 2021, 09:56:02

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

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

From

Ranier Vilela

Date:

12 February 2021, 19:08:02

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

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

From

Michael Paquier

Date:

13 February 2021, 05:40:23

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

Attachment

signature.asc

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

From

Zhihong Yu

Date:

13 February 2021, 07:09:17

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:

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

Attachment

reorder-buffer-base-snapshot.patch

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

From

Ranier Vilela

Date:

13 February 2021, 23:35:05

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

Attachment

reorderbuffer.patch

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

From

Ranier Vilela

Date:

13 February 2021, 23:40:38

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

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

From

Zhihong Yu

Date:

13 February 2021, 23:50:48

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 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

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

From

Ranier Vilela

Date:

13 February 2021, 23:59:32

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

Attachment

reorderbuffer_v3.patch

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

From

Zhihong Yu

Date:

14 February 2021, 00:12:25

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:

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

Attachment

reorder-buffer-v4.patch