public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ranier Vilela <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: Pg Hackers <[email protected]>
Subject: Re: Fix possible dereference null pointer (src/backend/replication/logical/reorderbuffer.c)
Date: Thu, 11 Apr 2024 10:37:20 -0300
Message-ID: <CAEudQAop7_XOPXwAozbyyugHw+Ca5Lur5hRo3SrusRLwV7n8Ng@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAEudQApm0Kffq4GKLdyogyW1tVbkHOOqWTWg169OTsm1W=FUHg@mail.gmail.com>
<[email protected]>
<CAEudQAp9auHMmBf-jS_-eaE+hQ6dNRJuBJmkGzrMtKndnOCQ9Q@mail.gmail.com>
<[email protected]>
Em qui., 11 de abr. de 2024 às 09:54, Heikki Linnakangas <[email protected]>
escreveu:
> On 11/04/2024 15:03, Ranier Vilela wrote:
> > Em qua., 10 de abr. de 2024 às 18:28, Heikki Linnakangas
> > <[email protected] <mailto:[email protected]>> escreveu:
> >
> > On 10/04/2024 21:07, Ranier Vilela wrote:
> > > Hi,
> > >
> > > Per Coverity.
> > >
> > > The function ReorderBufferTXNByXid,
> > > can return NULL when the parameter *create* is false.
> > >
> > > In the functions ReorderBufferSetBaseSnapshot
> > > and ReorderBufferXidHasBaseSnapshot,
> > > the second call to ReorderBufferTXNByXid,
> > > pass false to *create* argument.
> > >
> > > In the function ReorderBufferSetBaseSnapshot,
> > > fixed passing true as argument to always return
> > > a valid ReorderBufferTXN pointer.
> > >
> > > In the function ReorderBufferXidHasBaseSnapshot,
> > > fixed by checking if the pointer is NULL.
> >
> > If it's a "known subxid", the top-level XID should already have its
> > ReorderBufferTXN entry, so ReorderBufferTXN() should never return
> NULL.
> >
> > There are several conditions to not return NULL,
> > I think trusting never is insecure.
>
> Well, you could make it an elog(ERROR, ..) instead. But the point is
> that it should not happen, and if it does for some reason, that's very
> suprpising and there is a bug somewhere. In that case, we should *not*
> just blindly create it and proceed as if everything was OK.
>
Thanks for the clarification.
I will then suggest improving robustness,
but avoiding hiding any possible errors that may occur.
v1 patch attached.
best regards,
Ranier Vilela
Attachments:
[application/octet-stream] v1-fix-possible-dereference-null-pointer-reorderbuffer.patch (1.1K, ../CAEudQAop7_XOPXwAozbyyugHw+Ca5Lur5hRo3SrusRLwV7n8Ng@mail.gmail.com/3-v1-fix-possible-dereference-null-pointer-reorderbuffer.patch)
download | inline diff:
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..d0269fa913 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3144,8 +3144,12 @@ 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);
+ if (txn == NULL)
+ elog(ERROR, "unknown transaction, top-level xid: %u", txn->toplevel_xid);
+ }
Assert(txn->base_snapshot == NULL);
txn->base_snapshot = snap;
@@ -3471,8 +3475,13 @@ ReorderBufferXidHasBaseSnapshot(ReorderBuffer *rb, TransactionId xid)
/* a known subtxn? operate on top-level txn instead */
if (rbtxn_is_known_subxact(txn))
+ {
txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, false,
NULL, InvalidXLogRecPtr, false);
+ Assert(txn != NULL);
+ if (txn == NULL)
+ return false;
+ }
return txn->base_snapshot != NULL;
}
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Fix possible dereference null pointer (src/backend/replication/logical/reorderbuffer.c)
In-Reply-To: <CAEudQAop7_XOPXwAozbyyugHw+Ca5Lur5hRo3SrusRLwV7n8Ng@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox