Fix #68948: Revert "Fix bug #68532: convert.base64-encode omits padding ...

[slusarz]

...bytes"

This reverts commit 86f1875.

This is incorrect based on the current text of the PHP documentation
and, more important, broke backward compatibility with PHP < 5.5.21 and
< 5.6.5

[slusarz]

Note that this patch is for the 5.6 branch. The 5.5 branch needs the same reversion.

IMHO, there can be discussion about whether this is appropriate for PHP 7, but this is backward breaking for 5.5 and 5.6 (which now behave differently from 5.4).

[laruence]

@jpauli please have a look into this... I can not find the origin author of the ci which this PR is try to revert

[remicollet]

I confirm the new behavior introduce a small BC break (detected during 5.6.5RC phase) in the feof behavior (now, need a failed read to return true).

But this fix a issue with stream filter, and fix consistency between file stream, memory stream (and glibc file function).

This have break some tests in Guzzle (fixed upstream) and a Horde method (also fixed). Both upstream have agree the new behavior is correct, and the old Horde code was not correct.

[smalyshev]

@slusarz Could you add a test that includes the condition which produced a BC break? That would ensure it won't happen again in the future.

[slusarz]

@smalyshev Test added. Also added back the test for Bug 68532. So this fix means that Bug 68532 is broken again... but as I mentioned in ticket 68948 the correct solution is to fix the base64 filter, not to BC break something else (i.e. feof()) to fix. I will try to take a look at base64 filter to correctly fix this.

@remicollet You are exactly the opposite: the 68532 fix actually completely broke "consistency" between file stream and memory stream, at least in the PHP userland environment. Memory streams should behave like file-streams because, unlike network streams, the former both have defined lengths at any given point in time. But regardless, you can't break BC, which the original change did.

[slusarz]

FYI: I think I found the bug within the stream code that was not correctly calling the filter method with the closing parameter set to true, needed for the base64 encoder to flush its current buffered output. These commits were 0292cb8 and d263fea

Side note: without d263fea the output is correct but running one of the unit tests shows that the filter/bucket code in streams.c is being running something like 4,900 extra times (see https://travis-ci.org/php/php-src/jobs/53607863#L1882). Seems like a place that could potentially be investigated to remove those spurious calls.

[jails]

@slusarz does it mean https://bugs.php.net/bug.php?id=68481 will be broken again or your patch still preserve the correct feof() behavior ?

[slusarz]

Bug 68481 is incorrect. It suffers from the same mistakes as Bug 68532.

There is nothing wrong with the "old" (e.g. pre-5.5.21) feof() behavior. It's the way feof() has behaved in PHP for years, and I believe the way libc works for things like files.

Most important - the PHP documentation for feof() doesn't require "consistent" behavior across various streams (in fact, it explicitly states in a warning that feof() may behave differently depending on the underlying stream type). As I mentioned before, if feof() is desired to be "consistent" across all streams, then that is a perfectly valid policy decision (not one I necessarily agree with, but whatever...). But that's not a decision that can be made in the middle of point releases, especially since this doesn't fix the consistency problem - feof() for file streams behave differently than network streams so changing temporary streams doesn't change anything.

The issue in Bug 68532 was that the base64 stream filter was broken. This was what I fixed above (0292cb8, d263fea).

[jails]

So to sum things up with a 10 bytes "suff" we have the following behavior for feof():

For libc, HHVM (both memory streams and files) and PHP files the behavior is:

• reading 10 bytes leave eof to false
• reading 11 bytes set eof to true

However memory streams in PHP behave differently.

With PHP memory streams and the 86f1875 #68532 bugfix we have:

• reading 10 bytes leave eof to false
• reading 11 bytes leave eof to false too. And to have a valid eof value an extra read is necessary (which makes the feof() worst than the original implementation imo).

With the original PHP memory streams implementation we had:

• reading 10 bytes set eof to true directly
• reading 11 bytes set eof to true

Personnally I would prefer to have a consitent behavior over streams in PHP. @slusarz is there a use case where the original implementation can't be compatible with the "correct behavior" ? (I mean the one used by C & HHVM)

[jails]

Hope this will be fixed in PHP7. Adding new features is fine but it would be a lot more valuable to have this kind of core related issues fixed (FYI streams works as expected on HHVM).

[cmb69]

What's the status here?

[kaplanlior]

ping ?

[Tyrael]

sorry for forgetting about this one.
I'm convinced by @slusarz's reasoning about the original fix for 68532 was wrong, but I also think that @jails is right about feof() behavior should be consistent across file/memory stream.
maybe the best course of action would be merging this PR as-is into PHP-5.6 and upwards(as PHP-5.5 is extended support I'm not sure if @jpauli would want this fix in 5.5) and fix the consistency problem for master(PHP-7.0).

[kaplanlior]

Sounds like the right solution.

The important thing is that we at least have this fixed for 7.0, otherwise we'll have BC/consistency issues dragged for another release.

[jpauli]

5.5 won't be patched as this is not security related.

What does @remicollet think about proposed solution ?

[oerdnj]

JFTR Debian is already shipping the revert patch as it broke BC, so I would welcome a proper fix in 5.6 branch.

[jails]

Is this is going to be fixed in PHP 7 ?

[jails]

Wondering if someone can take a look at #1578 so we can move forward on this bug resolution ?

[krakjoe]

Since you're moving forward with the other PR, I'm closing this one ...

If I'm wrong to do that, apologies, just housework ;)