-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix GH-19685: Segfault when bzip2 filter has invalid parameters #19702
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -442,6 +442,10 @@ static php_stream_filter *php_bz2_filter_create(const char *filtername, zval *fi | |
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: only if you want, on master only maybe, making a goto label to cover the 3 cases.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is master now, probably a good idea to address this comment. |
||
| pefree(data->strm.next_out, persistent); | ||
| pefree(data, persistent); | ||
| return NULL; | ||
| } else { | ||
| blockSize100k = (int) blocks; | ||
| } | ||
|
|
@@ -452,6 +456,10 @@ static php_stream_filter *php_bz2_filter_create(const char *filtername, zval *fi | |
| zend_long work = zval_get_long(tmpzval); | ||
| if (work < 0 || work > 250) { | ||
| php_error_docref(NULL, E_WARNING, "Invalid parameter given for work factor (" ZEND_LONG_FMT ")", work); | ||
| pefree(data->strm.next_in, persistent); | ||
| pefree(data->strm.next_out, persistent); | ||
| pefree(data, persistent); | ||
| return NULL; | ||
| } else { | ||
| workFactor = (int) work; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| --TEST-- | ||
| GH-19685: bzip2.compress filter with invalid parameters should fail gracefully | ||
| --EXTENSIONS-- | ||
| bz2 | ||
| --FILE-- | ||
| <?php | ||
| $stream = fopen('php://memory', 'w+'); | ||
|
|
||
| // too low | ||
| $filter = stream_filter_append($stream, 'bzip2.compress', STREAM_FILTER_WRITE, array('blocks' => 0)); | ||
| var_dump($filter); | ||
|
|
||
| // too high | ||
| $filter = stream_filter_append($stream, 'bzip2.compress', STREAM_FILTER_WRITE, array('blocks' => 10)); | ||
| var_dump($filter); | ||
|
|
||
| // too low work | ||
| $filter = stream_filter_append($stream, 'bzip2.compress', STREAM_FILTER_WRITE, array('work' => -1)); | ||
| var_dump($filter); | ||
|
|
||
| // too high work | ||
| $filter = stream_filter_append($stream, 'bzip2.compress', STREAM_FILTER_WRITE, array('work' => 251)); | ||
| var_dump($filter); | ||
|
|
||
| fclose($stream); | ||
| ?> | ||
| --EXPECTF-- | ||
| Warning: stream_filter_append(): Invalid parameter given for number of blocks to allocate (0) in %s on line %d | ||
|
|
||
| Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d | ||
| bool(false) | ||
|
|
||
| Warning: stream_filter_append(): Invalid parameter given for number of blocks to allocate (10) in %s on line %d | ||
|
|
||
| Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d | ||
| bool(false) | ||
|
|
||
| Warning: stream_filter_append(): Invalid parameter given for work factor (-1) in %s on line %d | ||
|
|
||
| Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d | ||
| bool(false) | ||
|
|
||
| Warning: stream_filter_append(): Invalid parameter given for work factor (251) in %s on line %d | ||
|
|
||
| Warning: stream_filter_append(): Unable to create or locate filter "bzip2.compress" in %s on line %d | ||
| bool(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really fixing a bug tho?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's the format we use even for features. At least I've never seen a different format when referring to GitHub issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, but I can remove "bug" to indicate this fixes the Github issue itself if necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the format should remain the same. The format is even documented here: https://github.com/php/php-src/blob/master/CONTRIBUTING.md#news