Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:90924
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.160.171 as permitted sender)
MIME-Version: 1.0
Sender: yohgaki@gmail.com
In-Reply-To: <56A1054A.5080102@fedoraproject.org>
References: <03a501d15439$fcbf9ca0$f63ed5e0$@php.net> <56A1054A.5080102@fedoraproject.org>
Date: Tue, 26 Jan 2016 06:01:27 +0900
Message-ID: <CAGa2bXbavNd3H3+c_ASOBH6O6LoaBsj52pxPZmjTKZyssup7gw@mail.gmail.com>
To: Remi Collet <remi@fedoraproject.org>, julien pauli <jpauli@php.net>, 
	Ferenc Kovacs <tyrael@php.net>
Cc: "internals@lists.php.net" <internals@lists.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 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