Skip to content

Conversation

MagicalTux
Copy link
Contributor

@MagicalTux MagicalTux commented Aug 15, 2025

Resolving a potential use after free of the pgsql connection object.

See #19484 for details.

…stent connection is cleaned for future use
@MagicalTux MagicalTux requested a review from devnexen as a code owner August 15, 2025 06:03
@MagicalTux MagicalTux changed the title Fix #19484 by setting the notice processor to a no-op ext/pgsql: Fix #19484 by setting the notice processor to a no-op Aug 15, 2025

link = (PGconn *) rsrc->ptr;

/* unset notice processor */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

if (!PGG(ignore_notices)) {
    PQsetNoticeProcessor(link, _php_pgsql_notice_handler, NULL);
}

wdyt ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or possibly fetching the actual handler -> checking that is _php_pgsql_notice_handler -> unsetting it if yes

Copy link
Contributor Author

@MagicalTux MagicalTux Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ignore_notices (pgsql.ignore_notice without a s) is changed at runtime there is a risk we could have set the callback but fail to unset it. To be quite honest I am not sure if that's a risk at all, however setting it to a no-op should be safe either way.

The best way would be of course to check if we did set the callback by either storing a flag somewhere, or as an alternative check if the current callback is ours with:

if (PQsetNoticeProcessor(link, NULL, NULL) == _php_pgsql_notice_handler) {
    PQsetNoticeProcessor(link, _php_pgsql_notice_handler, NULL);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

or possibly fetching the actual handler -> checking that is _php_pgsql_notice_handler -> unsetting it if yes

@devnexen
Copy link
Member

devnexen commented Aug 15, 2025

Nice ! If you want, please credit yourself in NEWS by creating a PGSQL section for PHP 8.3.26 with Fixed bug GH-19484 ...

@devnexen devnexen linked an issue Aug 15, 2025 that may be closed by this pull request
@devnexen devnexen closed this in 987a3a5 Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pgsql: potential use after free when using persistent connections

2 participants