Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » PHP 7.0.3 RC1 is available for testing »

PHP 7.0.3 RC1 is available for testing

php.internals

Anatoliy Belsky

10 years ago
Hi, PHP 7.0.3 RC1 was just released and can be downloaded from: https://downloads.php.net/~ab/ The Windows binaries are available at http://windows.php.net/qa/ This release contains a number of bugfixes. For the list of bugfixes that you can target in your testing, please refer to the NEWS file: https://github.com/php/php-src/blob/php-7.0.3RC1/NEWS Please test it carefully, and report any bugs in the bug system. The stable release is planned for February 4th, if no critical issues will be discovered in the RC. Below is the verification information for the downloads: php-7.0.3RC1.tar.bz2 SHA256 hash: 7452f38e32288f9fcd18b4c385df76a13d9ac790f38d7e07ee4b5d3944bb9158 PGP signature: -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAABAgAGBQJWn1AWAAoJELyqMOqcDVdjSn4H/05Kz9NQwL4t8Sv3xIsJmmsw I0bmjP2S0L3MS+9nUWc+mhhtuVaHAJ8NI/wevKM0MKR+VcOxz8jVVQoO0Yehs0Wh 26jhNnKo1cXk31vq1alz5rdXoh8cvERsN0R7e4q3q4zTKmC7iRTa7tNOHWyI3NqL xNu51lulBE5PzBBBeOifM7TwTLIZieyhOrkBMwKeSKFNrLdFJYx4I9twppA3eeSe Kh//t0UH35QrFYsK86vhDN0KASDeqOUVxknAf34OdPRzfvNWSYM0vyOfuiE900qe PHzdoKjL12gdl6NRCgKDgjySIFc1dWtKoh03HSx9mtlPVtXOzFXU/0/tIk4XcoM= =h+FT -----END PGP SIGNATURE----- php-7.0.3RC1.tar.gz SHA256 hash: 7af732c88d1f4a63694d3d616aa92b0d6fb23cb5e7a4bf1e3a934d96f1c2580a PGP signature: -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAABAgAGBQJWn1AZAAoJELyqMOqcDVdjT4AH/0Wqg7WYgI4tBTh3NQh6f0qI ZkmtaF6zqMp1uqLOR2y1oIF20ZC3rpkDDsPFssazpenSgbFcHPC1BPadDGPVFd6p 3w9Jg/MLH0qbBhqMfnITx8WIHIc5HGC8GiElt9vQWfplRTN0Fvgty5fpEKMet+SK AutD6hXMrDWFb9KGE0e3vNP4M023vm6CRKklztZU30WTcgcGDsdeOsPppG0aPKJM OEvHjOu/tQavAVyoO4tyS48/Sfekml6BjEpiym/7b7jPFYm5XfbS2zosKSKgl2Jg KbIT1IeLJKGxmkAzc9JzYGesOMUei/dENnBdo4u0tUaa6f0va/Cnlyx7RPvKEJs= =XAGe -----END PGP SIGNATURE----- php-7.0.3RC1.tar.xz SHA256 hash: fbf475497d31d8a189ec1b61fe11912802e877476c74f072e8660322bf09e294 PGP signature: -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAABAgAGBQJWn1AbAAoJELyqMOqcDVdjagUH/0MhyDF42yDOKf5rB97Lj/V2 3PCusKIPJfb3AXijbYErH7OqktgsnyOmh+z3qkIHiejPLUGhSYryOgCPsIwkjG7O llFRDB9ExjQjq/MZOhhoTPvlrkPsW5pawIbUBYjNz6BY04xlE3hWezxBAp4iwyz+ yYyfGSCwtLAOL75ZhsHgCvNI5hvH+268W/7S1xhKaFyt3sSgFAiyBcF9J0MhxTbq 8kBWkl8DpUje19lQbLGEv9KkJQHC/OA+nLXK4BT1D9OfDCRMlJghcQyEGFyy79/0 flfLVLXv9mXuyZXDZUbS6iMmCZ5ceAMg4g+QKl5MtsXHcSv6EhaPEo14qWfYNcc= =UsDH -----END PGP SIGNATURE----- Thank you for your support. Anatol Belski and Ferenc Kovacs

Remi Collet

10 years ago
Le 21/01/2016 11:53, Anatol Belski a écrit :
> Hi, > > PHP 7.0.3 RC1 was just released and can be downloaded from:
Fedora detected a BC break in 5.6.18RC1 and 7.0.3RC1 related to session management. This update breaks: Horde_SessionHandler (2.2.6) and symfony (2.7.9) https://apps.fedoraproject.org/koschei/package/php-horde-Horde-SessionHandler + /usr/bin/phpunit . PHPUnit 4.8.17 by Sebastian Bergmann and contributors. .FE.ESPHP Warning: session_destroy(): Trying to destroy uninitialized session in /builddir/build/BUILD/php-horde-Horde-SessionHandler-2.2.6/Horde_SessionHandler-2.2.6/test/Horde/SessionHandler/Storage/BuiltinTest.php on line 115 ...........SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS........... Time: 231 ms, Memory: 5.00Mb There were 2 errors: 1) Horde_SessionHandler_Storage_BuiltinTest::testReopen Undefined index: sessiondata /builddir/build/BUILD/php-horde-Horde-SessionHandler-2.2.6/Horde_SessionHandler-2.2.6/test/Horde/SessionHandler/Storage/BuiltinTest.php:45 2) Horde_SessionHandler_Storage_BuiltinTest::testDestroy session_destroy(): Trying to destroy uninitialized session /builddir/build/BUILD/php-horde-Horde-SessionHandler-2.2.6/Horde_SessionHandler-2.2.6/test/Horde/SessionHandler/Storage/BuiltinTest.php:79
-- There was 1 failure: 1) Horde_SessionHandler_Storage_BuiltinTest::testRead Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'sessiondata|s:3:"foo";' +'' /builddir/build/BUILD/php-horde-Horde-SessionHandler-2.2.6/Horde_SessionHandler-2.2.6/test/Horde/SessionHandler/Storage/BuiltinTest.php:33 FAILURES! Tests: 36, Assertions: 84, Errors: 2, Failures: 1, Skipped: 37. PHP Warning: session_destroy(): Trying to destroy uninitialized session in /usr/share/pear/Horde/Cli.php on line 530 + /usr/bin/php -d include_path=.:/builddir/build/BUILDROOT/php-symfony-2.7.9-2.fc24.noarch/usr/share/php:/usr/share/php /usr/bin/phpunit --exclude-group benchmark,intl-data,tty --bootstrap bootstrap.php /builddir/build/BUILDROOT/php-symfony-2.7.9-2.fc24.noarch/usr/share/php/Symfony/Component/HttpKernel PHPUnit 4.8.17 by Sebastian Bergmann and contributors. .............................................FDumpDataCollectorTest.php on line 89: 123 ................. 63 / 440 ( 14%) ............................................................... 126 / 440 ( 28%) ............................................................... 189 / 440 ( 42%) ............................................................... 252 / 440 ( 57%) ............................................................... 315 / 440 ( 71%) ............................................................... 378 / 440 ( 85%) .............SSSSSSSSSSSSSSSSSS............................... Time: 17.34 seconds, Memory: 17.00Mb There was 1 failure: 1) Symfony\Component\HttpKernel\Tests\DataCollector\DumpDataCollectorTest::testCollectHtml Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ - <pre class=sf-dump id=sf-dump data-indent-pad=" "><a href="test:///builddir/build/BUILDROOT/php-symfony-2.7.9-2.fc24.noarch/usr/share/php/Symfony/Component/HttpKernel/Tests/DataCollector/DumpDataCollectorTest.php:89" title="/builddir/build/BUILDROOT/php-symfony-2.7.9-2.fc24.noarch/usr/share/php/Symfony/Component/HttpKernel/Tests/DataCollector/DumpDataCollectorTest.php"><span class=sf-dump-meta>DumpDataCollectorTest.php</span></a> on line <span class=sf-dump-meta>89</span>: + <pre class=sf-dump id=sf-dump data-indent-pad=" "><a href="" title=""><span class=sf-dump-meta></span></a> on line <span class=sf-dump-meta>89</span>: <span class=sf-dump-num>123</span> </pre> /builddir/build/BUILDROOT/php-symfony-2.7.9-2.fc24.noarch/usr/share/php/Symfony/Component/HttpKernel/Tests/DataCollector/DumpDataCollectorTest.php:118 FAILURES! Tests: 746, Assertions: 1310, Failures: 1, Skipped: 18. Legacy deprecation notices (11) >>>>>>>>>>>>>>>>>>>>>>> /builddir/build/BUILDROOT/php-symfony-2.7.9-2.fc24.noarch/usr/share/php/Symfony/Component/Intl + RET=1 Of course, both test suites were OK with 5.6.17. Remi.

Stas Malyshev

10 years ago
Hi!
> Fedora detected a BC break in 5.6.18RC1 and 7.0.3RC1 related to > session management.
Looks like something to do with last session changes. Yasuo, could you take a look at it?
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

10 years ago
Hi Remi and Stas, Thank you for head up. I'll address this today.
-- Yasuo Ohgaki yohgaki@ohgaki.net On Fri, Jan 22, 2016 at 3:44 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:

Yasuo Ohgaki

10 years ago
HI Remi and all, The reason of this BC I can think of immediately is location of GC. GC was performed after session read previously. GC is moved before session read because previous logic is buggy. Old behavior: - Do session read (Session data is initialized and store in memory) - Do session GC. (Previously read session data is deleted because it should be removed) - Obsolete session data is written and alive. New behavior: - Do session GC. (All obsolete data is removed) - Do session read (Obsolete data is gone. Initialize empty $_SESSION) - Newly created $_SESSION is used and obsolete session data is gone. I think the reported BC would be fixed by moving GC location in previous place. In fact, I had to fix phpt by the change. Old behavior may activate obsolete (should be deleted session). This is wrong. This could happen in low traffic sites more often. We have 2 choices - Enforce new and correct behavior since it will not affect normal operation. i.e. Actual production site - Restore old behavior for the time being. (For this option, I prefer to restore only for PHP 5.6, but Ok for PHP 7.0 also. No for master) Since current session module does not manage session data expiration precisely, restoring old behavior makes sense as we don't have to/cannot be strict about session data management anyway. Patch for fixing BC would be this, if I'm correct about the cause. e8f1c29cc96ce333fa808aba126297b77d94abdf (main patch) 57be57ac94ef46fa7a73889a1e7d6e75aa4ab00f (TSRM fix for PHP 5.6) I appreciate if you could run tests without these patches! Thanks. Any comments?
-- Yasuo Ohgaki yohgaki@ohgaki.net On Fri, Jan 22, 2016 at 1:20 AM, Remi Collet <remi@fedoraproject.org> wrote:

Stas Malyshev

10 years ago
Hi!
> We have 2 choices > - Enforce new and correct behavior since it will not affect normal > operation. i.e. Actual production site > - Restore old behavior for the time being. (For this option, I prefer > to restore only for PHP 5.6, but Ok for PHP 7.0 also. No for master)
Since it breaks applications, the existing behavior must be restored for both 5.6 and 7.0, unless it can be fixed ASAP. For master, a note should be made in UPGRADING at least, but if it's substantial change that requires applications to change then RFC would be requires I think.
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

10 years ago
Hi Stas, On Fri, Jan 22, 2016 at 7:15 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> >> We have 2 choices >> - Enforce new and correct behavior since it will not affect normal >> operation. i.e. Actual production site >> - Restore old behavior for the time being. (For this option, I prefer >> to restore only for PHP 5.6, but Ok for PHP 7.0 also. No for master) > > Since it breaks applications, the existing behavior must be restored for > both 5.6 and 7.0, unless it can be fixed ASAP. For master, a note should > be made in UPGRADING at least, but if it's substantial change that > requires applications to change then RFC would be requires I think.
The cause is logic, so it cannot be fixed unless we use bad logic. The fix will be included in my RFC ( https://wiki.php.net/rfc/precise_session_management ). BTW, it will not break application, but break tests for edge cases. I'll revert the patch if test result is good. Waiting Remi's response. Regards,
-- Yasuo Ohgakid yohgaki@ohgaki.net

Remi Collet

10 years ago
Le 21/01/2016 23:07, Yasuo Ohgaki a écrit :
> Patch for fixing BC would be this, if I'm correct about the cause. > > e8f1c29cc96ce333fa808aba126297b77d94abdf (main patch) > 57be57ac94ef46fa7a73889a1e7d6e75aa4ab00f (TSRM fix for PHP 5.6) > > I appreciate if you could run tests without these patches! Thanks.
Reverting both doesn't seems to fix the BC. PHPUnit 5.1.4 by Sebastian Bergmann and contributors. Runtime: PHP 5.6.19-dev Configuration: /work/GIT/horde/framework/SessionHandler/test/Horde/SessionHandler/phpunit.xml .FE.ES Warning: session_destroy(): Trying to destroy uninitialized session in /work/GIT/horde/framework/SessionHandler/test/Horde/SessionHandler/Storage/BuiltinTest.php on line 121 ...........SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS........... Time: 62 ms, Memory: 6.75Mb There were 2 errors: 1) Horde_SessionHandler_Storage_BuiltinTest::testReopen Undefined index: sessiondata /work/GIT/horde/framework/SessionHandler/test/Horde/SessionHandler/Storage/BuiltinTest.php:45 2) Horde_SessionHandler_Storage_BuiltinTest::testDestroy session_destroy(): Trying to destroy uninitialized session /work/GIT/horde/framework/SessionHandler/test/Horde/SessionHandler/Storage/BuiltinTest.php:79
-- There was 1 failure: 1) Horde_SessionHandler_Storage_BuiltinTest::testRead Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'sessiondata|s:3:"foo";' +'' /work/GIT/horde/framework/SessionHandler/test/Horde/SessionHandler/Storage/BuiltinTest.php:33

Remi Collet

10 years ago
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Reproducer attached It seems that using a user land SessionHandler, the "write" method is not called, raising this issue. Remi. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlaiBpgACgkQYUppBSnxahjMWACdHM/uJv8lVOSbTAUJT+rwWsC3 rcUAn1Mzd+n11xpuDI4bY5DIcqtt/Pu7 =Drkk -----END PGP SIGNATURE-----

Remi Collet

10 years ago
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Le 22/01/2016 11:38, Remi Collet a écrit :
> Reproducer attached > > It seems that using a user land SessionHandler, the "write" method > is not called, raising this issue.
Attached as .phpt Notice: the issue only happens after the: PHP Warning: session_start(): Cannot send session cookie - headers already sent by ... Remove the "Start" at the beginning and the test pass with 5.6.18RC1 Remi. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlaiHWYACgkQYUppBSnxahi1GQCgsaUt2tS20DH9Rd0+AkIwHqIC ErgAn2dJZ4pR0gWcCABj5EBbRECjGnsu =QB2l -----END PGP SIGNATURE-----

Stas Malyshev

10 years ago
Hi!
>> It seems that using a user land SessionHandler, the "write" method >> is not called, raising this issue. > > Attached as .phpt
Bisecting with this test shows a15e9ccba8a34553c029fb4574edba87c76447e5 as the breaking change. So we need to either fix it (this week, as next week it should be released) or revert this patch.
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

10 years ago
Hi all, On Wed, Jan 27, 2016 at 11:04 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> >>> It seems that using a user land SessionHandler, the "write" method >>> is not called, raising this issue. >> >> Attached as .phpt > > Bisecting with this test shows a15e9ccba8a34553c029fb4574edba87c76447e5 > as the breaking change. So we need to either fix it (this week, as next > week it should be released) or revert this patch.
Note for this issue. The change does not breaks normal codes as PHP cannot set new session ID when header is already sent. The session is _not_ accessible anyway. Not writing orphaned session does not matter at all. If you understand exactly what is happening, it is understandable this is not a BC for production code, but only breaks test code that inspect session behaviors and it found out bogus write() is gone now. Anyway, to fix existing app/framework unit test failures, (it's not fixing anything but leave bogus write(), though) removing php_session_abort() from php_session_cache_limitter() should be enough. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago
Hi all, On Wed, Jan 27, 2016 at 12:19 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> > On Wed, Jan 27, 2016 at 11:04 AM, Stanislav Malyshev > <smalyshev@gmail.com> wrote: >> >>>> It seems that using a user land SessionHandler, the "write" method >>>> is not called, raising this issue. >>> >>> Attached as .phpt >> >> Bisecting with this test shows a15e9ccba8a34553c029fb4574edba87c76447e5 >> as the breaking change. So we need to either fix it (this week, as next >> week it should be released) or revert this patch. > > Note for this issue. > The change does not breaks normal codes as PHP cannot set new session > ID when header is already sent. The session is _not_ accessible > anyway. Not writing orphaned session does not matter at all. > > If you understand exactly what is happening, it is understandable this > is not a BC for production code, but only breaks test code that > inspect session behaviors and it found out bogus write() is gone now. > > Anyway, to fix existing app/framework unit test failures, (it's not > fixing anything but leave bogus write(), though) > removing php_session_abort() from php_session_cache_limitter() should be enough.
One additional note. Removing the php_session_abort() will result in session_start() return TRUE even when the session is unusable. This may results in hard to debug bug why session is lost in certain pages. Anyway, I don't have much opinion to restore old behavior including restoring crashes by save handler abuse for PHP 5.6. I think PHP 7.0 is better to keep the patch as is, but it may be fixed in PHP 7.1. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago
Hi all, On Wed, Jan 27, 2016 at 12:28 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>> Note for this issue. >> The change does not breaks normal codes as PHP cannot set new session >> ID when header is already sent. The session is _not_ accessible >> anyway. Not writing orphaned session does not matter at all. >> >> If you understand exactly what is happening, it is understandable this >> is not a BC for production code, but only breaks test code that >> inspect session behaviors and it found out bogus write() is gone now. >> >> Anyway, to fix existing app/framework unit test failures, (it's not >> fixing anything but leave bogus write(), though) >> removing php_session_abort() from php_session_cache_limitter() should be enough. > > One additional note. > > Removing the php_session_abort() will result in session_start() return > TRUE even when the session is unusable. This may results in hard to > debug bug why session is lost in certain pages. > > Anyway, I don't have much opinion to restore old behavior including > restoring crashes by save handler abuse for PHP 5.6. I think PHP 7.0 > is better to keep the patch as is, but it may be fixed in PHP 7.1.
Sorry for scattered message, but it would be the best resolution to ask those frameworks/apps developer to ignore/remove the test? Returning FALSE for invalid session_start() should worth for frameworks/apps developer rather than keeping bogus test passing. IMO. Any comments?
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

10 years ago
Hi!
> Note for this issue. > The change does not breaks normal codes as PHP cannot set new session > ID when header is already sent. The session is _not_ accessible > anyway. Not writing orphaned session does not matter at all.
So it looks like this particular breakage is because write does not happen when cookies could not be sent. However, it is not necessarily true that session is not accessible in this case - session ID can be passed by other means, not only via cookies, and it also may be existing session ID. The second part of this test opens session with known ID, but write still does not happen. I think assuming that if cookie sending failed then the whole session failed is dangerous. Also, removing session write is a pretty serious change, I'm not sure this should be happening in a stable version.
> If you understand exactly what is happening, it is understandable this > is not a BC for production code, but only breaks test code that > inspect session behaviors and it found out bogus write() is gone now.> > Anyway, to fix existing app/framework unit test failures, (it's not > fixing anything but leave bogus write(), though) > removing php_session_abort() from php_session_cache_limitter() should be enough.
Yes, this patch: https://gist.github.com/smalyshev/4d8435b7993bef80c0ed fixes it for me. Remi, could you check that it also fixes the failures in the test suites?
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

10 years ago
Hi Stas, On Wed, Jan 27, 2016 at 2:45 PM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>> Note for this issue. >> The change does not breaks normal codes as PHP cannot set new session >> ID when header is already sent. The session is _not_ accessible >> anyway. Not writing orphaned session does not matter at all. > > So it looks like this particular breakage is because write does not > happen when cookies could not be sent. However, it is not necessarily > true that session is not accessible in this case - session ID can be > passed by other means, not only via cookies, and it also may be existing > session ID. The second part of this test opens session with known ID, > but write still does not happen. > I think assuming that if cookie sending failed then the whole session > failed is dangerous. Also, removing session write is a pretty serious > change, I'm not sure this should be happening in a stable version.
It's correct. I ignored trasid because of severe security bug in it that you should know of. (This is for others. We've been talking about the fix off this list. Please don't use transid or use it with extreme care with URL outputs. It's broken for a long time.) So correct behavior would be if transid is enabled and use_only_cookies is disabled, write session data. So the code should be if (!PS(use_only_cookies)) { php_session_abort() } Good point!
>> If you understand exactly what is happening, it is understandable this >> is not a BC for production code, but only breaks test code that >> inspect session behaviors and it found out bogus write() is gone now.> >> Anyway, to fix existing app/framework unit test failures, (it's not >> fixing anything but leave bogus write(), though) >> removing php_session_abort() from php_session_cache_limitter() should be enough. > > Yes, this patch: https://gist.github.com/smalyshev/4d8435b7993bef80c0ed > fixes it for me. Remi, could you check that it also fixes the failures > in the test suites?
Thank you for verifying it. So question would be if we are going to revert part that breaks unit tests (or even discard all patch that returns correct session_start() return value and save handler abuse crashes), or add necessary "if" statement to return correct value. One could argue if cookie cannot be sent, it should fail, but I think it's OK to return TRUE if session could persist via transid. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Anatol Belski

10 years ago
Hi Yasuo,
> -----Original Message----- > From: yohgaki@gmail.com [mailto:yohgaki@gmail.com] On Behalf Of Yasuo > Ohgaki > Sent: Wednesday, January 27, 2016 7:26 AM > To: Stanislav Malyshev <smalyshev@gmail.com> > Cc: Remi Collet <remi@fedoraproject.org>; internals@lists.php.net; Yasuo > Ohgaki <yohgaki@php.net> > Subject: Re: [PHP-DEV] PHP 7.0.3 RC1 is available for testing - **** BC break *** > > Hi Stas, > > On Wed, Jan 27, 2016 at 2:45 PM, Stanislav Malyshev <smalyshev@gmail.com> > wrote: > >> Note for this issue. > >> The change does not breaks normal codes as PHP cannot set new session > >> ID when header is already sent. The session is _not_ accessible > >> anyway. Not writing orphaned session does not matter at all. > > > > So it looks like this particular breakage is because write does not > > happen when cookies could not be sent. However, it is not necessarily > > true that session is not accessible in this case - session ID can be > > passed by other means, not only via cookies, and it also may be > > existing session ID. The second part of this test opens session with > > known ID, but write still does not happen. > > I think assuming that if cookie sending failed then the whole session > > failed is dangerous. Also, removing session write is a pretty serious > > change, I'm not sure this should be happening in a stable version. > > It's correct. I ignored trasid because of severe security bug in it that you should > know of. (This is for others. We've been talking about the fix off this list. Please > don't use transid or use it with extreme care with URL outputs. It's broken for a > long time.) > > So correct behavior would be if transid is enabled and use_only_cookies is > disabled, write session data. So the code should be > > if (!PS(use_only_cookies)) { > php_session_abort() > } > > Good point! > > >> If you understand exactly what is happening, it is understandable > >> this is not a BC for production code, but only breaks test code that > >> inspect session behaviors and it found out bogus write() is gone > >> now.> Anyway, to fix existing app/framework unit test failures, (it's > >> not fixing anything but leave bogus write(), though) removing > >> php_session_abort() from php_session_cache_limitter() should be enough. > > > > Yes, this patch: > > https://gist.github.com/smalyshev/4d8435b7993bef80c0ed > > fixes it for me. Remi, could you check that it also fixes the failures > > in the test suites? > > Thank you for verifying it. > > So question would be if we are going to revert part that breaks unit tests (or > even discard all patch that returns correct session_start() return value and save > handler abuse crashes), or add necessary "if" > statement to return correct value. One could argue if cookie cannot be sent, it > should fail, but I think it's OK to return TRUE if session could persist via transid. >
Thanks for all the investigation (as well Remi, Stas and everyone). At first glance last week, as for me, it looked like OK to keep at least the 7.0 part, as the breach was only concerning the unit tests but unlikely the actual web functionality. While it is good to have improvements and hardening on the unclean behaviors, the area is critical and should be kept stable in stable branches. Right now, it seems that the patch can have unexposed impacts. I would like also to remind that we're now quite short in time as finals are planned for the next week. With this in mind, IMHO it's better to play safe and revert 5.6 and 7.0 to the previous state before this change. Or at least, don't release these patches in the upcoming finals, but keep improving and fixing BC breaches in dev branch and re-evaluate the status in the next possible RC. Regards Anatol

Remi Collet

10 years ago
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Le 27/01/2016 06:45, Stanislav Malyshev a écrit :
> Yes, this patch: > https://gist.github.com/smalyshev/4d8435b7993bef80c0ed fixes it for > me. Remi, could you check that it also fixes the failures in the > test suites?
I confirm it fix the unit test about missing write, The Horde_SessionHandler test suite still fail differently: Both are related to "getSessionIDs" (which list active sessions) => PHP-5.6.18 (the branch) => PHP-5.6.18 + patch $ /work/build/php56/sapi/cli/php /usr/bin/phpunit - --include-path=/usr/share/pear . PHPUnit 5.1.4 by Sebastian Bergmann and contributors. .....F............SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS..FSS..... Time: 91 ms, Memory: 7.00Mb There were 2 failures: 1) Horde_SessionHandler_Storage_BuiltinTest::testGc Failed asserting that two arrays are equal. - --- Expected +++ Actual @@ @@ Array ( + 0 => 'db620868d98d974c1193d14c3ad7bb35' ) /usr/share/tests/pear/Horde_SessionHandler/Horde/SessionHandler/Storage/ BuiltinTest.php:98 2) Horde_SessionHandler_Storage_Sql_Pdo_SqliteTest::testList Failed asserting that two arrays are equal. - --- Expected +++ Actual @@ @@ Array ( - - 0 => 'sessionid' - - 1 => 'sessionid2' ) /usr/share/tests/pear/Horde_SessionHandler/Horde/SessionHandler/Storage/ Base.php:43 /usr/share/tests/pear/Horde_SessionHandler/Horde/SessionHandler/Storage/ Sql/Base.php:50 FAILURES! Tests: 35, Assertions: 77, Failures: 2, Skipped: 38. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlaoZHMACgkQYUppBSnxahjQ5ACgyLn2ySBmjW9O8Wc6Hbo0zCmR 4ToAoKH4Xgn8iqizrZrHhTOGDhgr+ABU =gL3l -----END PGP SIGNATURE-----

Yasuo Ohgaki

10 years ago
Hi Remi, On Wed, Jan 27, 2016 at 3:32 PM, Remi Collet <remi@fedoraproject.org> wrote:
> > There were 2 failures: > > 1) Horde_SessionHandler_Storage_BuiltinTest::testGc > Failed asserting that two arrays are equal. > - --- Expected > +++ Actual > @@ @@ > Array ( > + 0 => 'db620868d98d974c1193d14c3ad7bb35' > )
This sounds like php_session_gc() location correction patch is the cause. https://gist.github.com/yohgaki/c3783b22bae9dcb78d69 This patch allows expired session being active again, but this may work.
> > /usr/share/tests/pear/Horde_SessionHandler/Horde/SessionHandler/Storage/ > BuiltinTest.php:98 > > 2) Horde_SessionHandler_Storage_Sql_Pdo_SqliteTest::testList > Failed asserting that two arrays are equal. > - --- Expected > +++ Actual > @@ @@ > Array ( > - - 0 => 'sessionid' > - - 1 => 'sessionid2' > )
I have no idea for this now.
> > /usr/share/tests/pear/Horde_SessionHandler/Horde/SessionHandler/Storage/ > Base.php:43 > /usr/share/tests/pear/Horde_SessionHandler/Horde/SessionHandler/Storage/ > Sql/Base.php:50
Will fedora23's "dnf install php-horde*" install horde tests also? Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

10 years ago
Hi!
> The Horde_SessionHandler test suite still fail differently: Both > are related to "getSessionIDs" (which list active sessions)
Any way to get a short repro script for that too? In general, I start to be more and more doubtful about the idea of messing with sessions in stable versions.
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

10 years ago
Hi Remi, On Fri, Jan 22, 2016 at 7:38 PM, Remi Collet <remi@fedoraproject.org> wrote:
> > It seems that using a user land SessionHandler, the "write" method is > not called, raising this issue.
Thank you. I'll check it soon. (in a few days) I'm not sure what's causing it now, but I'll find it out. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago
Hi Remi, On Mon, Jan 25, 2016 at 11:47 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> > On Fri, Jan 22, 2016 at 7:38 PM, Remi Collet <remi@fedoraproject.org> wrote: >> >> It seems that using a user land SessionHandler, the "write" method is >> not called, raising this issue. > > Thank you. I'll check it soon. (in a few days)
Thank you for the script. I've tried and checked the output, but it seems OK to me. It call open->read->write->close, twice. -------------------------------------------------------- // Write session_name('sessionname'); session_id('sessionid'); session_start(); // This should call open/read $_SESSION['sessiondata'] = 'foo'; session_write_close(); // This should call write/close echo '------------------'.PHP_EOL; // Reopen session_write_close(); // Copy & paste error? Does not need this line and should not // call handlers while session is not active. Otherwise, // something wrong could happen. session_name('sessionname'); session_id('sessionid'); session_start(); // This should call open/read var_dump($_SESSION); // Shutdown should call write/close -------------------------------------------------------- As I add comments in code, session_write_close() should not do anything but the test script assumes write/close is called? I've tested with Fedora 23's PHP 5.6.17 and got the expected result -------------------------------------------------------- "MySessionHandler::__construct" string(22) "MySessionHandler::open" string(4) "/tmp" string(11) "sessionname" string(22) "MySessionHandler::read" string(9) "sessionid" string(23) "MySessionHandler::write" string(9) "sessionid" string(22) "sessiondata|s:3:"foo";" string(23) "MySessionHandler::close" ------------------ string(22) "MySessionHandler::open" string(4) "/tmp" string(11) "sessionname" string(22) "MySessionHandler::read" string(9) "sessionid" array(1) { ["sessiondata"]=> string(3) "foo" } string(23) "MySessionHandler::write" string(9) "sessionid" string(22) "sessiondata|s:3:"foo";" string(23) "MySessionHandler::close" -------------------------------------------------------- Could you give more details or other test script? Thank you!
-- Yasuo Ohgaki yohgaki@ohgaki.net

Remi Collet

10 years ago
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Le 25/01/2016 05:06, Yasuo Ohgaki a écrit :
> As I add comments in code, session_write_close() should not do > anything but the test script assumes write/close is called? I've > tested with Fedora 23's PHP 5.6.17 and got the expected result
Yes, The issue in with 5.6.18RC1 and 7.0.3RC1 Remi -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlalyNAACgkQYUppBSnxahgXXwCgpyFDZGBLflHLfAC0hBmKfiQq VkYAnigA73IdNuQu0yMrDPTmHwkqRflB =FHAr -----END PGP SIGNATURE-----

Anatol Belski

10 years ago
Hi,
> -----Original Message----- > From: Remi Collet [mailto:remi@fedoraproject.org] > Sent: Thursday, January 21, 2016 5:20 PM > To: internals@lists.php.net > Subject: Re: [PHP-DEV] PHP 7.0.3 RC1 is available for testing - **** BC break *** > + /usr/bin/php -d > include_path=.:/builddir/build/BUILDROOT/php-symfony-2.7.9- > 2.fc24.noarch/usr/share/php:/usr/share/php > /usr/bin/phpunit --exclude-group benchmark,intl-data,tty --bootstrap > bootstrap.php /builddir/build/BUILDROOT/php-symfony-2.7.9- > 2.fc24.noarch/usr/share/php/Symfony/Component/HttpKernel > PHPUnit 4.8.17 by Sebastian Bergmann and contributors. > .............................................FDumpDataCollectorTest.php on line 89: > 123 > ................. 63 / 440 ( 14%) > ............................................................... 126 / > 440 ( 28%) > ............................................................... 189 / > 440 ( 42%) > ............................................................... 252 / > 440 ( 57%) > ............................................................... 315 / > 440 ( 71%) > ............................................................... 378 / > 440 ( 85%) > .............SSSSSSSSSSSSSSSSSS............................... > Time: 17.34 seconds, Memory: 17.00Mb > There was 1 failure: > 1) > Symfony\Component\HttpKernel\Tests\DataCollector\DumpDataCollectorTest:: > testCollectHtml > Failed asserting that two strings are identical. > --- Expected > +++ Actual > @@ @@ > - <pre class=sf-dump id=sf-dump data-indent-pad=" "><a > href="test:///builddir/build/BUILDROOT/php-symfony-2.7.9- > 2.fc24.noarch/usr/share/php/Symfony/Component/HttpKernel/Tests/DataColle > ctor/DumpDataCollectorTest.php:89" > title="/builddir/build/BUILDROOT/php-symfony-2.7.9- > 2.fc24.noarch/usr/share/php/Symfony/Component/HttpKernel/Tests/DataColle > ctor/DumpDataCollectorTest.php"><span > class=sf-dump-meta>DumpDataCollectorTest.php</span></a> on line <span > class=sf-dump-meta>89</span>: > + <pre class=sf-dump id=sf-dump data-indent-pad=" "><a href="" > title=""><span class=sf-dump-meta></span></a> on line <span > class=sf-dump-meta>89</span>: > <span class=sf-dump-num>123</span> > </pre> > /builddir/build/BUILDROOT/php-symfony-2.7.9- > 2.fc24.noarch/usr/share/php/Symfony/Component/HttpKernel/Tests/DataColle > ctor/DumpDataCollectorTest.php:118 > FAILURES! > Tests: 746, Assertions: 1310, Failures: 1, Skipped: 18. > Legacy deprecation notices (11) > >>>>>>>>>>>>>>>>>>>>>>> > /builddir/build/BUILDROOT/php-symfony-2.7.9- > 2.fc24.noarch/usr/share/php/Symfony/Component/Intl > + RET=1 >
The fails in Symfony seems to be caused by ff7ed9021cd72a7f82dd4301cdc266afdff458ad, but also the corresponding places in Symfony seems a bit odd with %f format for a string https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php#L259 So that might be caused by both. Regards Anatol

Anatol Belski

10 years ago
Hi Julien,
> -----Original Message----- > From: Anatol Belski [mailto:anatol.php@belski.net] > Sent: Friday, January 22, 2016 9:29 AM > To: 'Remi Collet' <remi@fedoraproject.org>; internals@lists.php.net > Cc: julienpauli@gmail.com; 'Nicolas Grekas' <nicolas.grekas+php@gmail.com> > Subject: RE: [PHP-DEV] PHP 7.0.3 RC1 is available for testing - **** BC break ***
> The fails in Symfony seems to be caused by > ff7ed9021cd72a7f82dd4301cdc266afdff458ad, but also the corresponding > places in Symfony seems a bit odd with %f format for a string > > https://github.com/symfony/symfony/blob/master/src/Symfony/Component/H > ttpKernel/DataCollector/DumpDataCollector.php#L259 >
Were you able to investigate further on the strip_tags() issue? Or should we revert it for now? Thanks anatol

Nicolas Grekas

10 years ago
Hi, I can check early next week, I'm almost AFK currently. Thanks, Nicolas

Yasuo Ohgaki

10 years ago
Hi Remi and all, On Fri, Jan 22, 2016 at 1:20 AM, Remi Collet <remi@fedoraproject.org> wrote:
> Fedora detected a BC break in 5.6.18RC1 and 7.0.3RC1 related to > session management. > > This update breaks: > > Horde_SessionHandler (2.2.6) and symfony (2.7.9)
Thank you for notifying issue. Remi provided reproduceble phpt for this test failure and found out the cause of test failure. I really appreciate your help! Anyway, the cause was bug fix for "session_start() returns TRUE even when failures". Details for this specific case is following. Newer session cache limiter has php_session_abort() static int php_session_cache_limiter(TSRMLS_D) /* {{{ */ { php_session_cache_limiter_t *lim; if (PS(cache_limiter)[0] == '\0') return 0; if (PS(session_status) != php_session_active) return -1; if (SG(headers_sent)) { const char *output_start_filename = php_output_get_start_filename(TSRMLS_C); int output_start_lineno = php_output_get_start_lineno(TSRMLS_C); php_session_abort(TSRMLS_C); <<<<<<<<<< This is the cause if (output_start_filename) { php_error_docref(NULL TSRMLS_CC, E_WARNING, "Cannot send session cache limiter - headers already sent (output started at %s:%d)", output_start_filename, output_start_lineno); } else { php_error_docref(NULL TSRMLS_CC, E_WARNING, "Cannot send session cache limiter - headers already sent"); } return -2; } This is added because when session cannot be started, then it should fail. This fix is related to https://bugs.php.net/bug.php?id=71243 The php_session_abort() is not directly related to this bug, but this (and other fixes) is added because session_start() returns TRUE even when it fails/ should fail. Note: PHP 5.6's session_start() return value fix is not perfect to keep save handler compatibility which is a big one. PHP7 should return FALSE for session_start() failures always by the fix. Fixing the broken test should be just removing the php_session_abort() from php_session_cache_limiter(). The reason why it aborts is because it does not make sense to create and/or write session data even when client does not get the session ID. Note: session ID cookie is sent only when it is needed. i.e. New session. I think it does not matter much for session module & users if we remove the php_session_abort() or not. PHP 5.6's session_start() returns TRUE for session read error which is fatal, anyway. Being strict for this case does not worth much. Whichever is perfectly OK to me for PHP 5.6. For PHP 7.0, I would like to keep stricter behavior, but do not care much. How we handle this is up to release managers, since this fix is valid one even if it broke test on failure case. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Pierre Joye

10 years ago
On Jan 26, 2016 4:02 AM, "Yasuo Ohgaki" <yohgaki@ohgaki.net> wrote:
> > Hi Remi and all, > > On Fri, Jan 22, 2016 at 1:20 AM, Remi Collet <remi@fedoraproject.org>
wrote:
> > Fedora detected a BC break in 5.6.18RC1 and 7.0.3RC1 related to > > session management. > > > > This update breaks: > > > > Horde_SessionHandler (2.2.6) and symfony (2.7.9) > > Thank you for notifying issue. > Remi provided reproduceble phpt for this test failure and > found out the cause of test failure. I really appreciate your help! > > Anyway, the cause was bug fix for "session_start() returns > TRUE even when failures". Details for this specific case is following. > > Newer session cache limiter has php_session_abort() > > static int php_session_cache_limiter(TSRMLS_D) /* {{{ */ > { > php_session_cache_limiter_t *lim; > > if (PS(cache_limiter)[0] == '\0') return 0; > if (PS(session_status) != php_session_active) return -1; > > if (SG(headers_sent)) { > const char *output_start_filename = > php_output_get_start_filename(TSRMLS_C); > int output_start_lineno = php_output_get_start_lineno(TSRMLS_C); > > php_session_abort(TSRMLS_C); <<<<<<<<<< This is the cause > if (output_start_filename) { > php_error_docref(NULL TSRMLS_CC, E_WARNING, "Cannot send > session cache limiter - headers already sent (output started at > %s:%d)", output_start_filename, output_start_lineno); > } else { > php_error_docref(NULL TSRMLS_CC, E_WARNING, "Cannot send > session cache limiter - headers already sent"); > } > return -2; > } > > This is added because when session cannot be started, then it should fail. > This fix is related to https://bugs.php.net/bug.php?id=71243 > The php_session_abort() is not directly related to this bug, but this (and > other fixes) is added because session_start() returns TRUE even when it
fails/
> should fail. > > Note: PHP 5.6's session_start() return value fix is not perfect to keep > save handler compatibility which is a big one. PHP7 should return FALSE > for session_start() failures always by the fix. > > Fixing the broken test should be just removing the php_session_abort()
from
> php_session_cache_limiter().
Fixing broken tests most likely mean BC will remain which is not so good. I understand the overall goal to improve session security but this is an area that has behaved this way for years. I am totally convinced that such big changes should have (or should) in stable branches, be 7.0 or 5.6. Especially because testing these changes take time. Cheers, Pierre

Ferenc Kovacs

10 years ago
> > > > This is added because when session cannot be started, then it should > fail. > > This fix is related to https://bugs.php.net/bug.php?id=71243 > > The php_session_abort() is not directly related to this bug, but this > (and > > other fixes) is added because session_start() returns TRUE even when it > fails/ > > should fail. > > > > Note: PHP 5.6's session_start() return value fix is not perfect to keep > > save handler compatibility which is a big one. PHP7 should return FALSE > > for session_start() failures always by the fix. > > > > Fixing the broken test should be just removing the php_session_abort() > from > > php_session_cache_limiter(). > > Fixing broken tests most likely mean BC will remain which is not so good. >
you probably meant BC *break* will remain which I agree that isn't good.
> I understand the overall goal to improve session security but this is an > area that has behaved this way for years. I am totally convinced that such > big changes should have (or should) in stable branches, be 7.0 or 5.6. > Especially because testing these changes take time. >
I have to look through the changes and the original bugreport which warranted this change but my gut feeling is that this shouldn't be changed in a micro version and Yasuo even changed/fixed a handful of tests together of the code changes, so the potential impact could be even bigger than what Remi spotted with their CI pipeline.