Horton

Thread

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

    Алена Васильева <gorcom2012@gmail.com> — 2025-09-18T08:44:57Z

    Hello!
I propose adding an Assert at this point as a way to document the contract
between this code and its environment.
This was previously discussed in BUG #18785 in pqsql-bugs
https://www.postgresql.org/message-id/flat/a6oxxee6blexicuark46yydtaqulsjvkrwkri6aic4vbofjxse%404a6j4kuwda7u#c52de413b182d9c42fe1eb34a82871b5

Best regards,
Eugeny Goryachev


Patch for PostgreSQL 16.6:
---
 src/backend/storage/buffer/bufmgr.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/backend/storage/buffer/bufmgr.c
b/src/backend/storage/buffer/bufmgr.c
index e066a3f888f..1d3b31f5070 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -895,6 +895,17 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
        bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
    }

+   /*
+    * Validate the contract between flags and bmr.rel.
+    *
+    * EB_CREATE_FORK_IF_NEEDED implies we need to take extension lock,
+    * which requires a valid relation. Conversely, if we're skipping
+    * the extension lock (e.g., in recovery), then we must not be creating
+    * a fork here.
+    */
+   Assert(!(flags & EB_CREATE_FORK_IF_NEEDED) || !(flags &
EB_SKIP_EXTENSION_LOCK));
+   Assert(bmr.rel != NULL || (flags & EB_SKIP_EXTENSION_LOCK));
+
    /*
     * If desired, create the file if it doesn't exist.  If
     * smgr_cached_nblocks[fork] is positive then it must exist, no need for
-- 
2.42.4

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

    Nikolay Shaplov <dhyan@nataraj.su> — 2025-10-20T11:14:29Z

    В письме от четверг, 18 сентября 2025 г. 11:44:57 Москва, стандартное время 
пользователь Алена Васильева написал:

> I propose adding an Assert at this point as a way to document the contract
> between this code and its environment.
> This was previously discussed in BUG #18785 in pqsql-bugs
> https://www.postgresql.org/message-id/flat/a6oxxee6blexicuark46yydtaqulsjvkr
> wkri6aic4vbofjxse%404a6j4kuwda7u#c52de413b182d9c42fe1eb34a82871b5

Hi!

I'd like to join reviewing process. I am already familiar with this issue from 
previous discussions.

PROS: 
This issue as I know have been found while applying static analyzer to 
PostgreSQL code. This is not a bug per ce, everything works as expected, there 
is certain calling convention that makes dubious situation unreachable.

But what static analyzer is actually reporting here: there is some calling 
convention that have not been explicitly declared, and this might lead to 
problems in future: somebody breaks this convention, but we will never know 
about it. 

From my point of view, the best way to declare calling convention in 
PostgreSQL are Asserts + human readable comments explaining these Asserts.

This will stop static analyzer from raising false alarm, and these Asserts 
will raise a true alarm if some developer tries to break that convention. And 
comments near the Assert will hopefully help him understanding what is wrong 
here.

So it is win-win situation: we improve code quality, and future users of 
static analyzers are no longer bored with false alarms.

So in general this patch should be acceppted

CONS:
But this patch application should be redo from scratch.

1. If you want to submit patch, add it to the commit fest. Not only post it on 
the mailing list.

2. Patch should be attach to a letter, not embedded in message text. 
Commitfest robot will not be able to extract it from there

3. To prepare patch you should use `git format-patch` command. And please add 
please create a prototype of a commit message that commiter will use to 
creating final one, he will use while committing this patch. Do not make 
committer invent it from scratch :-)

You can find more guides for patch submitting via these links:
https://wiki.postgresql.org/wiki/Submitting_a_Patch

https://peter.eisentraut.org/blog/2023/05/09/how-to-submit-a-patch-by-email-2023-edition

4.

> +   /*
> +    * Validate the contract between flags and bmr.rel.
> +    *

This "validate" word might accidentally confuse someone. Because Asserts in 
postgres clearly do no validation, they are completely removed from code in 
production builds, so no checks are done. Asserts are more a way of setting 
traps for future call convention violator,s that will be triggered while 
running tests, fuzzing or any other non-production activities that includes 
turning Asserts on. So "valudate" is a bad word here....


So please submit patch accordingly and I will be a reviewer for it.


-- 
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su