Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:90966
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.160.182 as permitted sender)
MIME-Version: 1.0
Sender: yohgaki@gmail.com
In-Reply-To: <56A85967.4090603@gmail.com>
References: <03a501d15439$fcbf9ca0$f63ed5e0$@php.net> <56A1054A.5080102@fedoraproject.org>
 <CAGa2bXaKaoiSKLpodHAEKurSEwrs9rYO25uwqGdbwg_KKUkGbg@mail.gmail.com>
 <56A2069B.2050007@fedoraproject.org> <56A21D68.6030403@fedoraproject.org>
 <56A825A9.9020706@gmail.com> <CAGa2bXYf9JFtH_yqjYfL7UiqDCzxmeFy9s-Ja4YPF+UjxDs3=w@mail.gmail.com>
 <56A85967.4090603@gmail.com>
Date: Wed, 27 Jan 2016 15:25:49 +0900
Message-ID: <CAGa2bXb_3eM2EaM=Zt2Mf-pLnGDvgek2M87O_7mL5f6HLp3s2w@mail.gmail.com>
To: Stanislav Malyshev <smalyshev@gmail.com>
Cc: Remi Collet <remi@fedoraproject.org>, 
	"internals@lists.php.net" <internals@lists.php.net>, Yasuo Ohgaki <yohgaki@php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] PHP 7.0.3 RC1 is available for testing - **** BC break ***
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

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