Fix #74267: segfault with streams and invalid data#5686
Closed
cmb69 wants to merge 2 commits intophp:PHP-7.3from
Closed
Fix #74267: segfault with streams and invalid data#5686cmb69 wants to merge 2 commits intophp:PHP-7.3from
cmb69 wants to merge 2 commits intophp:PHP-7.3from
Conversation
If the current character is a line break character, it cannot be a tab or space character, so we would always fail with an invalid sequence error. Obviously, these conditions are meant to be exclusive.
Member
|
This change looks right to me, but it's not immediately clear how it leads to a segfault. From a cursory reading, it would just caused an invalid sequence error. |
Member
Author
|
The segfaults happens in the final call to that function, because the following condition is false then (lb_cnt==1, lb_ptr==0): php-src/ext/standard/filters.c Line 1019 in ceae816 but shortly after in_pp and in_left_p (both being NULL) are dereferenced. It seems to me that the additional constraint (&& lb_cnt == lb_ptr) is not correct, but even if I remove it, there would be three warnings regarding invalid sequences, although there are only two invalid sequences in the test.
|
Member
|
@cmb69 Shouldn't we fix that condition as well? It's not clear to me that there is no other code path that could run into the same issue. |
If `in_pp == NULL || in_left_p == NULL` is true, we hit a segfault if we are not returning right away. Therefore, the additional constraints don't make sense.
Member
Author
|
Yes, you're right. These constraints don't make sense. |
nikic
approved these changes
Jun 8, 2020
Member
Author
|
Thanks! Applied as 12c59f6. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
If the current character is a line break character, it cannot be a tab
or space character, so we would always fail with an invalid sequence
error. Obviously, these conditions are meant to be exclusive.