Re: Fix possible dereference null pointer (src/backend/replication/logical/reorderbuffer.c)

Em sáb., 13 de abr. de 2024 às 04:16, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
escreveu:

> On 11/04/2024 16:37, Ranier Vilela wrote:
> > Em qui., 11 de abr. de 2024 às 09:54, Heikki Linnakangas
> > <hlinnaka(at)iki(dot)fi <mailto:hlinnaka(at)iki(dot)fi>> escreveu:
> >
> > On 11/04/2024 15:03, Ranier Vilela wrote:
> > > Em qua., 10 de abr. de 2024 às 18:28, Heikki Linnakangas
> > > <hlinnaka(at)iki(dot)fi <mailto:hlinnaka(at)iki(dot)fi> <mailto:hlinnaka(at)iki(dot)fi
> > <mailto:hlinnaka(at)iki(dot)fi>>> 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.
>
> I don't much like adding extra runtime checks for "can't happen"
> scenarios either. Assertions would be more clear, but in this case the
> code would just segfault trying to dereference the NULL pointer, which
> is fine for a "can't happen" scenario.
>
This sounds a little confusing to me.
Is the project policy to *tolerate* dereferencing NULL pointers?
If this is the case, no problem, using Assert would serve the purpose of
protecting against these errors well.

> Looking closer, when we identify an XID as a subtransaction, we:
> - assign toplevel_xid,
> - set RBTXN_IS_SUBXACT, and
> - assign toptxn pointer.
>
> ISTM the 'toplevel_xid' and RBTXN_IS_SUBXACT are redundant.
> 'toplevel_xid' is only used in those two calls that do
> ReorderBufferTXNByXid(rb, txn->toplevel_xid,...), and you could replace
> those by following the 'toptxn' pointer directly. And RBTXN_IS_SUBXACT
> is redundant with (toptxn != NULL). So here's a patch to remove
> 'toplevel_xid' and RBTXN_IS_SUBXACT altogether.
>
+ if (rbtxn_is_subtxn(txn))
+ txn = rbtxn_get_toptxn(txn);

rbtxn_get_toptxn already calls rbtxn_is_subtx,
which adds a little unnecessary overhead.
I made a v1 of your patch, modifying this, please ignore it if you don't
agree.

best regards,
Ranier Vilela