Обсуждение: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
От
Ranier Vilela
Дата:
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;
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
Вложения
Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
От
Kyotaro Horiguchi
Дата:
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)
От
Ranier Vilela
Дата:
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 inIt does not make sense to generate a SEGV on purpose and
> 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.
assertion would not solve why this happens in a production environment.
Better to report an error if the second call fails.
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)
От
Michael Paquier
Дата:
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:
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
Вложения
Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)
От
Ranier Vilela
Дата:
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)
От
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.")));
+ (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)
От
Ranier Vilela
Дата:
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:
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