Re: BUG #18785: Pointer bmr.rel, dereferenced by passing as 1st parameter to function is checked for NULL later

Hi,

On 2025-01-28 15:04:35 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2025-01-28 13:58:13 +0000, PG Bug reporting form wrote:
> >> Hello, I suggest the following patch for this issue.
> >>
> >> @@ -905,6 +905,8 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
> >> bmr.smgr->smgr_cached_nblocks[fork] == InvalidBlockNumber)
> >> &&
> >> !smgrexists(bmr.smgr, fork))
> >> {
> >> +
> >> +               Assert(bmr.rel != NULL);
> >>                 LockRelationForExtension(bmr.rel, ExclusiveLock);
>
> > I guess it couldn't hurt to add them. It's fine for existing callers...
>
> Seems quite pointless really.  If bmr.rel is NULL, the
> LockRelationForExtension call will SIGSEGV all by itself.
> Transforming that into a SIGABRT isn't moving the football much.

Well, the assertions I suggested would catch the buggy code even if, for the
current call, the relation fork *does* exist. For LockRelationForExtension()
to crash would require actually reaching the block...


> The actually interesting question is whether it's possible to
> reach there with bmr.rel being NULL, and if so what we have to do
> to avoid such a crash.  Adding an Assert still doesn't help.

Pretty sure it can't be reached.  The current user of bmr.rel == NULL is
recovery, where we normally don't have a relcache. That doesn't use
EB_CREATE_FORK_IF_NEEDED. The users (freespacemap, visibilitymap) of
EB_CREATE_FORK_IF_NEEDED all pass the relation in and would crash themselves
if called with a NULL rel.

Greetings,

Andres Freund