Skip to content

ext/standard: Adds and modifies XFAIL tests for filter bucket leaks - #23440

Open
NickSdot wants to merge 1 commit into
php:masterfrom
NickSdot:tests/sfr-xfail-warn
Open

ext/standard: Adds and modifies XFAIL tests for filter bucket leaks#23440
NickSdot wants to merge 1 commit into
php:masterfrom
NickSdot:tests/sfr-xfail-warn

Conversation

@NickSdot

Copy link
Copy Markdown
Contributor

I wanted to fix two warn: XFAIL section but test passes but found #20058 which already attempts fixing the leaks; don't want to open an overlapping one. However, the PR is already a bit older -- currently builds without leak detection warn because XFAIL is too broad. So here we change to use SKIPIF to "xfail", "xleak", or skip only where relevant so that we for the time being have no warnings where no leaks can be detected and tests pass. Additionally, two tests for related issues that surfaced were added (also mentioned in #20058 (comment)) .

Should this target 8.4 (20058 still targets 8.3)?

cc @Girgias the two modified tests were added by you, are you okay with this change?
cc @iliaal would you want to look at the two added tests?

@iliaal

iliaal commented Sep 4, 2026

Copy link
Copy Markdown
Member

#23267 is the master version of this; GH-20058 is 8.3. It already touches both files you re-gated and just drops their XFAIL, so rebasing on it removes those two hunks.

On the new tests, measured on a debug NTS master build: ..._reentrant is already clean under #23267, so its SKIPIF will start emitting warn: XFAIL section but test passes as soon as that lands. unprocessed_buckets still leaks 3 allocations under it, because the abandoned bucket sits on $out and _php_stream_filter_flush() never drains it. That one is #23564, on 8.4. So neither GH-20058 nor #23267 belongs in its comment.

The block also ends in an unconditional die('skip requires a leak detector'), so a release build skips both new tests and nothing anywhere asserts the warning texts or the Handled warning ordering. Assert the output unconditionally and gate only the leak.

22 lines of SKIPIF copied into four files is heavy for something you want to delete again shortly. The rest of the tree gates these with if (getenv('SKIP_ASAN')) die('xleak ...');.

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.

2 participants