Horton

Thread

Commits

  1. test_custom_stats: Fail if loading module outside shared_preload_libraries

  1. Prevent crash when calling pgstat functions with unregistered stats kind

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-07-01T07:19:49Z

    Hi hackers,

While reviewing [1], I got segfault(s) because I created a custom statistics
extension that I forgot to add to shared_preload_libraries. Then using one of
its function produced:

"
Core was generated by `postgres: postgres postgres [local] SELECT                                    '.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  pgstat_init_entry (kind=kind@entry=24, shhashent=shhashent@entry=0x73f6c341a740) at pgstat_shmem.c:335
335             chunk = dsa_allocate_extended(pgStatLocal.dsa,
"

Indeed, if a custom statistics extension is loaded via CREATE EXTENSION without
being listed in shared_preload_libraries, its _PG_init() skips the call to
pgstat_register_kind(). The SQL functions are still created, and calling them
invokes pgstat functions with a kind that was never registered.

pgstat_get_kind_info() returns NULL in this case. The existing code only
checked this via Assert() in some paths, so non-assert builds would dereference
NULL and segfault.

The attached patch adds runtime checks in all public-facing pgstat functions that
accept a PgStat_Kind and dereference the returned kind info:

- pgstat_prep_pending_entry()
- pgstat_fetch_entry()
- pgstat_reset()
- pgstat_reset_of_kind()
- pgstat_have_entry()
- pgstat_snapshot_fixed()
- pgstat_init_entry()
- pgstat_reset_entry()

Each now raises ERROR with ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when
the kind is not known or registered.

[1]: https://postgr.es/m/akSi2txzLZWQL31Q%40bdtpg

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  2. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Ewan Young <kdbase.hack@gmail.com> — 2026-07-01T08:20:39Z

    Hi Bertrand,

On Wed, Jul 1, 2026 at 3:20 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi hackers,
>
> While reviewing [1], I got segfault(s) because I created a custom statistics
> extension that I forgot to add to shared_preload_libraries. Then using one of
> its function produced:
>
> "
> Core was generated by `postgres: postgres postgres [local] SELECT                                    '.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  pgstat_init_entry (kind=kind@entry=24, shhashent=shhashent@entry=0x73f6c341a740) at pgstat_shmem.c:335
> 335             chunk = dsa_allocate_extended(pgStatLocal.dsa,
> "
>
> Indeed, if a custom statistics extension is loaded via CREATE EXTENSION without
> being listed in shared_preload_libraries, its _PG_init() skips the call to
> pgstat_register_kind(). The SQL functions are still created, and calling them
> invokes pgstat functions with a kind that was never registered.
>
> pgstat_get_kind_info() returns NULL in this case. The existing code only
> checked this via Assert() in some paths, so non-assert builds would dereference
> NULL and segfault.
>
> The attached patch adds runtime checks in all public-facing pgstat functions that
> accept a PgStat_Kind and dereference the returned kind info:
>
> - pgstat_prep_pending_entry()
> - pgstat_fetch_entry()
> - pgstat_reset()
> - pgstat_reset_of_kind()
> - pgstat_have_entry()
> - pgstat_snapshot_fixed()
> - pgstat_init_entry()
> - pgstat_reset_entry()
>
> Each now raises ERROR with ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when
> the kind is not known or registered.

Thanks for the patch — nice catch, and the diagnosis looks right.

One small thing: in pgstat_snapshot_fixed(), the existing
Assert(pgstat_is_kind_valid(kind)); becomes redundant after the new NULL
check. A non-NULL kind_info already implies the kind is valid (that's the
only way pgstat_get_kind_info() returns non-NULL), so the assert can never
fire. Might as well drop it and keep just the fixed_amount one.

>
> [1]: https://postgr.es/m/akSi2txzLZWQL31Q%40bdtpg
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com

-- 
Regards,
Ewan Young

  3. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-07-02T03:27:18Z

    Hi Ewan,

On Wed, Jul 01, 2026 at 04:20:39PM +0800, Ewan Young wrote:
> Thanks for the patch — nice catch, and the diagnosis looks right.

Thanks for looking at it!

> One small thing: in pgstat_snapshot_fixed(), the existing
> Assert(pgstat_is_kind_valid(kind)); becomes redundant after the new NULL
> check. A non-NULL kind_info already implies the kind is valid (that's the
> only way pgstat_get_kind_info() returns non-NULL), so the assert can never
> fire. Might as well drop it and keep just the fixed_amount one.

Yeah good catch, done in the attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  4. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Michael Paquier <michael@paquier.xyz> — 2026-07-02T03:43:43Z

    On Thu, Jul 02, 2026 at 03:27:18AM +0000, Bertrand Drouvot wrote:
> On Wed, Jul 01, 2026 at 04:20:39PM +0800, Ewan Young wrote:
>> One small thing: in pgstat_snapshot_fixed(), the existing
>> Assert(pgstat_is_kind_valid(kind)); becomes redundant after the new NULL
>> check. A non-NULL kind_info already implies the kind is valid (that's the
>> only way pgstat_get_kind_info() returns non-NULL), so the assert can never
>> fire. Might as well drop it and keep just the fixed_amount one.
> 
> Yeah good catch, done in the attached.

I am not convinced that it is worth bothering in the core code about
this class of failures; they are just not interesting, and impossible
to miss.

It seems to me that this error is in the _PG_init() of the modules in
modules/test_custom_stats/: we should not bypass the
pgstat_register_kind() if not loading the library from
shared_preload_libraries, but let the call happen and fail.
--
Michael

  5. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-07-02T04:06:01Z

    Hi,

On Thu, Jul 02, 2026 at 12:43:43PM +0900, Michael Paquier wrote:
> On Thu, Jul 02, 2026 at 03:27:18AM +0000, Bertrand Drouvot wrote:
> > On Wed, Jul 01, 2026 at 04:20:39PM +0800, Ewan Young wrote:
> >> One small thing: in pgstat_snapshot_fixed(), the existing
> >> Assert(pgstat_is_kind_valid(kind)); becomes redundant after the new NULL
> >> check. A non-NULL kind_info already implies the kind is valid (that's the
> >> only way pgstat_get_kind_info() returns non-NULL), so the assert can never
> >> fire. Might as well drop it and keep just the fixed_amount one.
> > 
> > Yeah good catch, done in the attached.
> 
> I am not convinced that it is worth bothering in the core code about
> this class of failures; they are just not interesting, and impossible
> to miss.
> 
> It seems to me that this error is in the _PG_init() of the modules in
> modules/test_custom_stats/: we should not bypass the
> pgstat_register_kind() if not loading the library from
> shared_preload_libraries, but let the call happen and fail.

I agree that the responsibility should primarily be in the extension. However,
the issue is that the NULL dereference happens inside core code (pgstat_prep_pending_entry,
etc.), and the resulting segfault(s) cause the postmaster to terminate all
backends (not just the offending session).

Given that one misconfigured extension can crash all connections on the server,
a defensive check in core seems reasonable (kind of similar to 341e9a05e7b).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  6. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Michael Paquier <michael@paquier.xyz> — 2026-07-02T04:10:18Z

    On Thu, Jul 02, 2026 at 04:06:01AM +0000, Bertrand Drouvot wrote:
> I agree that the responsibility should primarily be in the extension. However,
> the issue is that the NULL dereference happens inside core code (pgstat_prep_pending_entry,
> etc.), and the resulting segfault(s) cause the postmaster to terminate all
> backends (not just the offending session).
> 
> Given that one misconfigured extension can crash all connections on the server,
> a defensive check in core seems reasonable (kind of similar to 341e9a05e7b).

Nope, this was a different thing, doable in a couple of steps:
- Load the library.
- Write custom stats.
- Stop the server, flush the stats.
- Edit the configuration, not loading the library.
- Restart the server, loading failed.

The problem of this thread ought to be blocked at its source, in the
extension itself: let's not give free hands to an extension to do what
it should not be allowed to do.  There is a similar defense in
test_custom_rmgrs, as one example.  We should just map to that.
--
Michael

  7. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-07-02T04:23:16Z

    Hi,

On Thu, Jul 02, 2026 at 01:10:18PM +0900, Michael Paquier wrote:
> On Thu, Jul 02, 2026 at 04:06:01AM +0000, Bertrand Drouvot wrote:
> > I agree that the responsibility should primarily be in the extension. However,
> > the issue is that the NULL dereference happens inside core code (pgstat_prep_pending_entry,
> > etc.), and the resulting segfault(s) cause the postmaster to terminate all
> > backends (not just the offending session).
> > 
> > Given that one misconfigured extension can crash all connections on the server,
> > a defensive check in core seems reasonable (kind of similar to 341e9a05e7b).
> 
> Nope, this was a different thing, doable in a couple of steps:
> - Load the library.
> - Write custom stats.
> - Stop the server, flush the stats.
> - Edit the configuration, not loading the library.
> - Restart the server, loading failed.
> 
> The problem of this thread ought to be blocked at its source, in the
> extension itself: let's not give free hands to an extension to do what
> it should not be allowed to do.  There is a similar defense in
> test_custom_rmgrs, as one example.  We should just map to that.

Ok but what about extensions that don't call pgstat_register_kind() at all? Your
point is that they would see the issue during the development of the extension? (If
so, I think I could agree).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  8. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-07-02T04:43:32Z

    On Thu, Jul 02, 2026 at 04:23:16AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Thu, Jul 02, 2026 at 01:10:18PM +0900, Michael Paquier wrote:
> > On Thu, Jul 02, 2026 at 04:06:01AM +0000, Bertrand Drouvot wrote:
> > > I agree that the responsibility should primarily be in the extension. However,
> > > the issue is that the NULL dereference happens inside core code (pgstat_prep_pending_entry,
> > > etc.), and the resulting segfault(s) cause the postmaster to terminate all
> > > backends (not just the offending session).
> > > 
> > > Given that one misconfigured extension can crash all connections on the server,
> > > a defensive check in core seems reasonable (kind of similar to 341e9a05e7b).
> > 
> > Nope, this was a different thing, doable in a couple of steps:
> > - Load the library.
> > - Write custom stats.
> > - Stop the server, flush the stats.
> > - Edit the configuration, not loading the library.
> > - Restart the server, loading failed.
> > 
> > The problem of this thread ought to be blocked at its source, in the
> > extension itself: let's not give free hands to an extension to do what
> > it should not be allowed to do.  There is a similar defense in
> > test_custom_rmgrs, as one example.  We should just map to that.
> 
> Ok but what about extensions that don't call pgstat_register_kind() at all? Your
> point is that they would see the issue during the development of the extension? (If
> so, I think I could agree).

Something like in the attached?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  9. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Michael Paquier <michael@paquier.xyz> — 2026-07-02T05:00:06Z

    On Thu, Jul 02, 2026 at 04:43:32AM +0000, Bertrand Drouvot wrote:
> On Thu, Jul 02, 2026 at 04:23:16AM +0000, Bertrand Drouvot wrote:
>> Ok but what about extensions that don't call pgstat_register_kind() at all? Your
>> point is that they would see the issue during the development of the extension? (If
>> so, I think I could agree).

Extensions doing custom stats have to call the register API, or
they're broken.  This is the same assumption as custom RMGRs.  There
are many ways to break the backend if you don't know what you do, just
take hooks for example.  That's just one of them.

> Something like in the attached?

Yes, that looks OK here.
--
Michael

  10. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-07-02T05:02:36Z

    Hi,

On Thu, Jul 02, 2026 at 02:00:06PM +0900, Michael Paquier wrote:
> On Thu, Jul 02, 2026 at 04:43:32AM +0000, Bertrand Drouvot wrote:
> > On Thu, Jul 02, 2026 at 04:23:16AM +0000, Bertrand Drouvot wrote:
> >> Ok but what about extensions that don't call pgstat_register_kind() at all? Your
> >> point is that they would see the issue during the development of the extension? (If
> >> so, I think I could agree).
> 
> Extensions doing custom stats have to call the register API, or
> they're broken.  This is the same assumption as custom RMGRs.  There
> are many ways to break the backend if you don't know what you do, just
> take hooks for example.  That's just one of them.
> 
> > Something like in the attached?
> 
> Yes, that looks OK here.

That makes sense, I do agree.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  11. Re: Prevent crash when calling pgstat functions with unregistered stats kind

    Michael Paquier <michael@paquier.xyz> — 2026-07-02T06:53:50Z

    On Thu, Jul 02, 2026 at 05:02:36AM +0000, Bertrand Drouvot wrote:
> That makes sense, I do agree.

Done that now down to v19, thanks, in the shape of some pure code
deletion.
--
Michael