Skip to content

Fix ASAN reported leak in FPM config test #10296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Jan 12, 2023

This is an attempt to fix https://github.com/php/php-src/actions/runs/3843039843/jobs/6544956534 including some failures for tests running under root.

@bukka bukka changed the base branch from master to PHP-8.2 January 12, 2023 11:56
@bukka bukka force-pushed the fpm_config_test_exit_leak branch from ba6987a to aaf388c Compare January 12, 2023 11:57
@bukka
Copy link
Member Author

bukka commented Jan 12, 2023

I should note that the fix is not verified because I had some issues to recreate it locally. Tried with various versions of Clang (10, 12, 13, 14) but no luck. I also tried with image built from this Dockerfile https://github.com/bukka/php-util/blob/00c90767f30bed9cb5c4757203dd8cb23db4e64d/docker/sanitize-ubuntu-22-04.dockerfile specially created for this purpose but running following was also fine (no error):

TEST_FPM_RUN_AS_ROOT=1 php run-tests.php --asan sapi/fpm/tests/bug68591-conf-test-user.phpt     

Not sure if I'm doing anything wrong so any help to get it failing locally would be appreciated.

@devnexen
Copy link
Member

devnexen commented Jan 12, 2023

Would it help ?
php-fpm-asan-68591.txt

tried with clang 15 on debian unstable

@bukka
Copy link
Member Author

bukka commented Jan 12, 2023

@devnexen Are you able to get it insider Docker? I would just need a way to reliable recreate but no idea why it doesn't work there...

@bukka
Copy link
Member Author

bukka commented Jan 12, 2023

Otherwise if you have got some build instructions, that would be awesome too.

@devnexen
Copy link
Member

Otherwise if you have got some build instructions, that would be awesome too.

Nothing particular

./configure --enable-address-sanitizer --enable-debug --enable-fpm

@bukka
Copy link
Member Author

bukka commented Jan 12, 2023

Don't you need to disable pcre-jit and some other bits...?

Do you get the same for those options?

https://github.com/bukka/php-util/blob/00c90767f30bed9cb5c4757203dd8cb23db4e64d/docker/sanitize-ubuntu-22-04.dockerfile#L23-L51

@devnexen
Copy link
Member

I did not pass anymore configure options than those, wanted to have smallest reproductible build.
By any chance, don t you redirect asan failures on files (e.g. ASAN_OPTIONS) ?
otherwise I may have a look later.

@bukka
Copy link
Member Author

bukka commented Jan 12, 2023

Ok with those options I'm actually able to recreate it and can confirm that the fix actually fixes the issue. Thanks!

I just see that I was actually trying to test MSAN which was fine. Must have somehow misread the pipeline config...

@bukka bukka closed this in 1b48a5c Jan 12, 2023
@bukka
Copy link
Member Author

bukka commented Jan 12, 2023

Just a noted that I picked only 8.2+ just in case there are some strange scenarios when someone would expect no shutdown during the config test. Also the tests using config test are in 8.2 only.

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