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

Hi,

On 2025-01-28 13:58:13 +0000, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference: 18785
> Logged by: Daniel Elishakov
> Email address: dan-eli(at)mail(dot)ru
> PostgreSQL version: 16.6
> Operating system: ubuntu 20.04
> Description:

I don't think it's particularly useful to report missing assertions as bugs,
particularly not multiple bugs for basically the same issue.

> 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);
>
> /* could have been closed while waiting for lock */
>
>
> I think we need to check bmr.rel for NULL, because Asserts above do not
> suggest bmr.rel != NULL. Moreover, it is being checked for NULL later after
> being passed into function LockRelationForExtension(bmr.rel, ExclusiveLock);

I guess it couldn't hurt to add them. It's fine for existing callers...

The specific suggested assertion seems not great, because it'll often not even
be reached, due to the file not being empty.

I think it'd be better to assert something like

/* EB_CREATE_FORK_IF_NEEDED is currently only needed with extension lock */
Assert(!(flags & EB_CREATE_FORK_IF_NEEDED) || !(flags & EB_SKIP_EXTENSION_LOCK));

/* can only acquire extension lock if rel is passed in */
Assert(bmr.rel != NULL || (flags & EB_SKIP_EXTENSION_LOCK));

Greetings,

Andres Freund