Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:71770
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain bof.de designates 80.242.145.70 as permitted sender)
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: internals@lists.php.net
Date: Thu, 30 Jan 2014 05:41:41 +0100
Message-ID: <18069656.FoN8fZuO6C@rofl>
User-Agent: KMail/4.11.5 (Linux/3.13.0-k10-bof; KDE/4.11.5; x86_64; ; )
In-Reply-To: <CAGa2bXYn8i9R2+FGMUeyrr00MjRMug5QhheLa=gLKSOc+0j-CA@mail.gmail.com>
References: <CAGa2bXZEXx7H+atr3LK8OLLQGSd9a-6+Gey+RhPwP0d7EvTBmw@mail.gmail.com> <52E9A631.5050808@sugarcrm.com> <CAGa2bXYn8i9R2+FGMUeyrr00MjRMug5QhheLa=gLKSOc+0j-CA@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Subject: Re: [PHP-DEV] Re: [VOTE] Introduce session.lock, session.lazy_write and session.lazy_destory
From: php@bof.de (Patrick Schaaf)

Hello Yasuo,

First of all, the github "compare" link you give in the RFC shows a vast 
amount of unrelated changed files. Playing around a bit I find that comparing 
not to php:master but to yohgaki:PHP-5.6 gives a better overview:

https://github.com/yohgaki/php-src/compare/PHP-5.6...PHP-5.6-rfc-session-lock

Regarding minimize_lock:

Generally a really dangerous feature. minimize_lock sounds so friendly, does 
not imply danger really.

Alternative naming proposal: unlocked_thus_unsafe



The minimize_lock=true implementation in mod_files.c looks wrong:

1) in PS_READ_FUNC the flock(LOCK_EX) is placed after fstat(). A concurrent 
writer could rewrite the file between the fstat and the pread/read, resulting 
in either a short read or an incomplete read, in both cases resulting in 
invalid session data being read.

2) in PS_WRITE_FUNC the flock(LOCK_EX) is placed after ftruncate(). Thus, an 
ftruncate could hit both concurrent readers and writers at any time, resulting 
in various kinds of problems.

3) again in PS_WRITE_FUNC in the check whether to ftruncate, data->st_size, 
which was set in an earlier PS_READ_FUNC, is used for the comparison. However, 
the file on disk may have been changed by a concurrent writer in the meantime, 
and have an arbitrary different size, resulting in a wrong decision to 
ftruncate-or-not.

To solve all three problems, I'd suggest

- unconditional taking of the flock(LOCK_EX) in ps_files_open(), while keeping 
the LOCK_UN in PS_READ_FUNC/PS_WRITE_FUNC (if minimize_lock=true).
- also when minimize_lock=true, make an fstat() call in PS_WRITE_FUNC before 
the need-to-ftruncate check, ignoring data->st_size from the read.



Reading that code, also in the base PHP repository, I think I spotted an 
unrelated bug: In ps_files_open() in the not-WIN32-open_basedir fstat/S_ISLNK 
_error_ cases: data->fd is closed, but data->fd is then NOT set to -1.


best regards
  Patrick