pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18785: Pointer bmr.rel, dereferenced by passing as 1st parameter to function is checked for NULL later
4+ messages / 3 participants
[nested] [flat]
* BUG #18785: Pointer bmr.rel, dereferenced by passing as 1st parameter to function is checked for NULL later
@ 2025-01-28 13:58 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 4+ messages in thread
From: PG Bug reporting form @ 2025-01-28 13:58 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: dan-eli@mail.ru
The following bug has been logged on the website:
Bug reference: 18785
Logged by: Daniel Elishakov
Email address: dan-eli@mail.ru
PostgreSQL version: 16.6
Operating system: ubuntu 20.04
Description:
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);
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18785: Pointer bmr.rel, dereferenced by passing as 1st parameter to function is checked for NULL later
@ 2025-01-28 19:55 Andres Freund <andres@anarazel.de>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 4+ messages in thread
From: Andres Freund @ 2025-01-28 19:55 UTC (permalink / raw)
To: dan-eli@mail.ru; pgsql-bugs@lists.postgresql.org
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@mail.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
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18785: Pointer bmr.rel, dereferenced by passing as 1st parameter to function is checked for NULL later
@ 2025-01-28 20:04 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 4+ messages in thread
From: Tom Lane @ 2025-01-28 20:04 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: dan-eli@mail.ru; pgsql-bugs@lists.postgresql.org
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.
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.
regards, tom lane
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18785: Pointer bmr.rel, dereferenced by passing as 1st parameter to function is checked for NULL later
@ 2025-01-28 20:12 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 4+ messages in thread
From: Andres Freund @ 2025-01-28 20:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: dan-eli@mail.ru; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2025-01-28 20:12 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-01-28 13:58 BUG #18785: Pointer bmr.rel, dereferenced by passing as 1st parameter to function is checked for NULL later PG Bug reporting form <noreply@postgresql.org>
2025-01-28 19:55 ` Andres Freund <andres@anarazel.de>
2025-01-28 20:04 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-01-28 20:12 ` Andres Freund <andres@anarazel.de>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox