Fix GH-19685: Segfault when bzip2 filter has invalid parameters #19702
Conversation
alexandre-daubois
force-pushed
the
stream-append-bz2-failure
branch
from
e6d35d8 to
4c88e07
Compare
| zend_long blocks = zval_get_long(tmpzval); | ||
| if (blocks < 1 || blocks > 9) { | ||
| php_error_docref(NULL, E_WARNING, "Invalid parameter given for number of blocks to allocate (" ZEND_LONG_FMT ")", blocks); | ||
| pefree(data->strm.next_in, persistent); |
There was a problem hiding this comment.
nit: only if you want, on master only maybe, making a goto label to cover the 3 cases.
|
It should be ok in this case but breaking filter chain is sometimes not safe (might cause breaks) so I will need to review it properly - I should be properly looking to filters in 2 or 3 weeks time. |
There was a problem hiding this comment.
So I just spent some time to getting to know bzip2 extension to see if I'm missing anything by any chance but I'm not sure I understand what the point of this PR is.
You claim (in the PR title) that this is fixes the segfault but from the stacktrace in #19685 (comment), this looks to me that the fix for that is in #19708 because it's segfaulting due to trying to use ops write on data stream (that has got write ops NULL). That bug GH-19685 is really just a duplicate of GH-19705 - the only difference is that it's using bzip2 filter which also as got some buffered rubbish to write. Basically if you changed the blocks to 4 and work to 0, it would still segfault so this really doesn't fix anything and it's completely unrelated to that bug. In fact this is not a bug at all but just a BC break because currently it's just warning and then using defaults. But you changed it to not use defaults and fails. This should really go through deprecation.
So at least you need to change the branch, titles and description and then add it to the deprecation RFC for 8.6.
4 participants
Fix #19685
When
blocksorworkhave invalid values, it currently let the function execution continue. Instead, we should free some resources and return null to avoid the segfault.