Skip to content

Fix GH-11529: Crash after dealing with an Apache request#11530

Closed
ndossche wants to merge 1 commit into
php:masterfrom
ndossche:fix-apache-session
Closed

Fix GH-11529: Crash after dealing with an Apache request#11530
ndossche wants to merge 1 commit into
php:masterfrom
ndossche:fix-apache-session

Conversation

@ndossche
Copy link
Copy Markdown
Member

@ndossche ndossche commented Jun 25, 2023

In an MPM worker scenario we have 1 module, N threads. Each thread must have their globals initialised. If we only initialise the filename fields in MINIT, then the threads have an uninitialized value. If the uninitialized value is not NULL, this leads to segfaults upon access.

@ndossche
Fix phpGH-11529: Crash after dealing with an Apache request
8d43709 
In an MPM worker scenario we have 1 module, N threads. Each thread must
have their globals initialised. If we only initialise the filename
fields in MINIT, then the threads have an uninitialized value. If the
uninitialized value is not NULL, this leads to segfaults upon access.
@ndossche ndossche requested a review from Girgias as a code owner June 25, 2023 20:28
@ndossche ndossche linked an issue Jun 25, 2023 that may be closed by this pull request
Crash after dealing with an Apache request #11529
Closed
@ndossche
Copy link
Copy Markdown
Member Author

ndossche commented Jun 25, 2023

As a side note, it strikes me as weird that PS(session_status) = php_session_none; is done in MINIT, and also in GINIT. This commit introduced it: ada5c40 I don't know what this means though. Let's just leave it here...

@ndossche ndossche requested a review from iluuu1994 June 25, 2023 20:48
@drupol
Copy link
Copy Markdown
Contributor

drupol commented Jun 26, 2023

Trying the patch right now and will report back asap.

@drupol drupol mentioned this pull request Jun 26, 2023
Merged
12 tasks
@Girgias
Copy link
Copy Markdown
Member

Girgias commented Jun 26, 2023

As a side note, it strikes me as weird that PS(session_status) = php_session_none; is done in MINIT, and also in GINIT. This commit introduced it: ada5c40 I don't know what this means though. Let's just leave it here...

Apparently UMR means Uninitialized Memory Read

Girgias
Girgias approved these changes Jun 26, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

This looks sensible to me

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Jun 26, 2023

As a side note, it strikes me as weird that PS(session_status) = php_session_none; is done in MINIT, and also in GINIT. This commit introduced it: ada5c40 I don't know what this means though. Let's just leave it here...

Looking at the file at that commit, this looks like PHP 4 era code which doesn't seem to have a GINIT stage, so maybe that's why?

@drupol
Copy link
Copy Markdown
Contributor

drupol commented Jun 26, 2023 •
edited
Loading

I just ran the tests and it's still crashing. Looks like the patch hasn't fixed them yet. I'm going to dedicate a couple of hours in the evening on this and I'll let you know.

Patch is fine, tests are now passing! Thanks !!!

@ndossche
Copy link
Copy Markdown
Member Author

ndossche commented Jun 26, 2023

Thanks a lot for testing @drupol! This patch will be included in the next alpha.

@drupol
Copy link
Copy Markdown
Contributor

drupol commented Jun 26, 2023

Thanks to you !!! We are now ready to ship PHP 8.3.0alpha2 (+3 patches) in Nix :)

@ndossche
Copy link
Copy Markdown
Member Author

ndossche commented Jun 26, 2023

As a side note, it strikes me as weird that PS(session_status) = php_session_none; is done in MINIT, and also in GINIT. This commit introduced it: ada5c40 I don't know what this means though. Let's just leave it here...

Looking at the file at that commit, this looks like PHP 4 era code which doesn't seem to have a GINIT stage, so maybe that's why?

Ah that makes sense. I don't feel comfortable removing the line though since it's been there for a very long time, and one memory write extra doesn't hurt performance anyway so 🤷

@ndossche ndossche closed this in c0147a0 Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Girgias Girgias Girgias approved these changes

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees

No one assigned

Labels

Extension: session

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Crash after dealing with an Apache request

3 participants

@ndossche @drupol @Girgias