Skip to content
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

Fix #77069: stream filter loses final block of data #6001

Closed
wants to merge 4 commits into from

Conversation

cmb69
Copy link
Contributor

@cmb69 cmb69 commented Aug 17, 2020

Reading from a stream may return greater than zero, but nonetheless the
stream's EOF flag may have been set. We have to cater to this
condition by setting the close flag for filters.

@cmb69 cmb69 marked this pull request as draft August 17, 2020 16:14
@cmb69 cmb69 added the Bug label Aug 17, 2020
@cmb69
Copy link
Contributor Author

cmb69 commented Aug 27, 2020

I have just pushed a commit which fixes the so far failing ext/soap/tests/bug73037.phpt. The problem was that the zlib.deflate filter apparently assumed that when called with PSFS_FLAG_FLUSH_CLOSE it was always able to deflate all available data in one deflate() call. This is, of course, not possible if the output buffer is too small.

I wonder whether this assumption is a valid assumption for filters to make, because it is guaranteed the PSFS_FLAG_FLUSH_CLOSE flag will only ever be passed, if no further input data is to be processed. If that is the case, other filters might break due to the first commit of this PR as well, and PR #3320 would be the proper fix for https://bugs.php.net/79984, but it would not fix https://bugs.php.net/77069, which I still consider to be a duplicate of 79984 (or more precisely, 79984 is a dupe of 77069).

Could someone more proficient with the streams layer than I am please clarify?

@cmb69 cmb69 marked this pull request as ready for review August 27, 2020 10:49
@cmb69
Copy link
Contributor Author

cmb69 commented Sep 7, 2020

Maybe @sgolemon could have a look?

@cmb69 cmb69 requested a review from sgolemon September 20, 2020 21:08
Reading from a stream may return greater than zero, but nonetheless the
stream's EOF flag may have been set.  We have to cater to this
condition by setting the close flag for filters.
If `inflate()` is called with flush mode `Z_FINISH`, but the output
buffer is not large enough to inflate all available data, it fails with
`Z_BUF_ERROR`.  However, `Z_BUF_ERROR` is not fatal; in fact, the zlib
manual states: "If deflate returns with Z_OK or Z_BUF_ERROR, this
function must be called again with Z_FINISH and more output space
(updated avail_out) but no more input data, until it returns with
Z_STREAM_END or an error."  Hence, we do so.

This fixes the so far failing ext/soap/tests/bug73037.phpt.
@cmb69 cmb69 changed the base branch from PHP-7.3 to PHP-7.4 November 10, 2020 13:29
@cmb69
Copy link
Contributor Author

cmb69 commented Nov 10, 2020

I have rebased onto PHP-7.4, since it's a bit late for 7.3 fixes.

I still wonder whether there is an implicit assumption that the PSFS_FLAG_FLUSH_CLOSE flag will only ever be passed, if no further input data is to be processed.

@cmb69
Copy link
Contributor Author

cmb69 commented Nov 24, 2020

@nikic, any thought on this. /cc @fisharebest

@php-pulls php-pulls closed this in 65f5573 Dec 8, 2020
@cmb69 cmb69 deleted the cmb/77069 branch December 8, 2020 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant