Hi Yasuo, > -----Original Message----- > From: yohg...@gmail.com [mailto:yohg...@gmail.com] On Behalf Of Yasuo > Ohgaki > Sent: Wednesday, January 27, 2016 7:26 AM > To: Stanislav Malyshev <smalys...@gmail.com> > Cc: Remi Collet <r...@fedoraproject.org>; internals@lists.php.net; Yasuo > Ohgaki <yohg...@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 <smalys...@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
