Re: Re: [VOTE] Introduce session.lock, session.lazy_write and session.lazy_destory

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