Fix GH-10052: Browscap crashes PHP 8.1.12 on request shutdown (apache2) #10883
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…che2) get_browser() implements a lazy parse system for the browscap INI configuration. There are two possible moments when a browscap configuration can be loaded: during module startup or during request. In case of module startup, the strings are persistent strings, while for the request they are not. The INI parser must therefore know whether to create persistent or non-persistent strings. It does this by looking at CG(ini_parser_unbuffered_errors). If that value is 1 it's persistent, otherwise non-persistent. Note that this also controls how the errors are reported: if it's 1 then the errors are sent to stderr, otherwise we get E_WARNINGs. Currently, a hardcoded value of 1 is always used for that CG value in browscap_read_file(). This means we'll always create persistent strings *and* we'll not report parse errors correctly as E_WARNINGs. We fix both the crash and the lack of warnings by passing the value of persistent instead of a hardcoded 1. This is also in line with how other INI parsing code is called in ext/standard: they also make sure that during request a value of 0 is passed.
Girgias
approved these changes
Mar 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
get_browser() implements a lazy parse system for the browscap INI configuration. There are two possible moments when a browscap configuration can be loaded: during module startup or during request. In case of module startup, the strings are persistent strings, while for the request they are not.
The INI parser must therefore know whether to create persistent or non-persistent strings. It does this by looking at CG(ini_parser_unbuffered_errors). If that value is 1 it's persistent, otherwise non-persistent. Note that this also controls how the errors are reported: if it's 1 then the errors are sent to stderr, otherwise we get E_WARNINGs.
Currently, a hardcoded value of 1 is always used for that CG value in browscap_read_file(). This means we'll always create persistent strings and we'll not report parse errors correctly as E_WARNINGs. We fix both the crash and the lack of warnings by passing the value of persistent instead of a hardcoded 1.
This is also in line with how other INI parsing code is called in ext/standard: they also make sure that during request a value of 0 is passed.
I don't know how to write a test for this. I reproduced this bug and tested this fix manually, because we don't seem to have Apache SAPI tests.