PHP 5.6 rfc session lock #628

yohgaki · 2014-03-18T00:44:12Z

Implements https://wiki.php.net/rfc/session-lock-ini

* PHP-5.6: (281 commits) Updated NEWS Added new PDO::PGSQL_ATTR_DISABLE_PREPARES that uses PQexecParams Deprecated PDO::PGSQL_ATTR_DISABLE_NATIVE_PREPARED_STATEMENT Drop PDO support for extremely old libpq versions Fix compile error with theoretically supported versions of libcurl < 7.12.3. fix path separator Use /dev/urandom as the default mcrypt_create_iv() source Add tests for bug #66872 and gmp_[rem]root Fixed mcrypt test case Removed bogus loops restored the old code in 5.4/5 related to bug #66872 Fixed Bug #66875 (Improve performance of multi-row OCI_RETURN_LOB queries) updated libsqlite to 3.8.3.1 in 5.5 branch, too DI Switch from a single flag to a flag byte updated libmagic.patch updated libmagic.patch update NEWS fix #66872, invalid argument crashes gmp_testbit fix #66872, invalid argument crashes gmp_testbit ...

I need new internal API for this. I'll write one later. This reverts commit 6f99c0f.

smalyshev · 2014-03-18T05:39:04Z

ext/session/php_session.h

+		ps_delete_##x, ps_gc_##x, ps_create_sid_##x, \
+		php_session_validate_sid, php_session_update
+
+/* PHP 5.6 save handler module efinitions */


typo: should be "definitions"

.... Thank you

…de read_only acutually a read_only. ToDo: Bring back "lazy_write" option. Fix tests for new SessionUpdateTimestampHandler. Do more tests, especially for object based user save handlers.

ToDo: Bring back lazy_write option. Fix tests.

narfbg · 2014-03-19T00:27:22Z

ext/session/tests/save_handler.inc

@@ -20,6 +20,8 @@ function read($id) {
    $session_id = $id;
    echo "Read [${session_save_path},${id}]\n";
    $session_file = "$session_save_path/".SESSION_FILE_PREFIX.$id;
+    // read MUST create file. Otherwise, strict mode will not work


Why is this in read() at all?
open() is made for opening the file descriptor, so IMO strict mode should be handled in there via a file_exists check.

In general, open will not create session data. For example, connecting to DB wouldn't create any.

Ok ...
Btw, this is something that I've stumbled upon multiple times - read() is only expected to return a string. What if there was a failure?

When there is read error, it should return FALSE.
However, the code handles error in session module is commented out, since there are too many save handlers that return FALSE for empty read which should NOT be an error.

I'll document how to write correct save handler. There are others like this.

From what I saw, it's commented out because it throws errors instead of handling silently ...
IMO, this can be handled by session_start() returning FALSE if open() or read() fails.

Sure, although I don't understand why a save handler would return FALSE for an empty read.
But if that's not handled ... what happens on a real failure?

We may consider to have proper error raised/returned for PHP7. Currently, we don't have proper documentation for writing correct save handler.

When real error happened, it's ignored :)

Alternatively, user may raise error/exception and catch it.

Yes, error handling in general currently sucks, so that's a must for PHP++.
But ignoring a real error is really bad for something as sensitive as sessions ... if it was up to me, I'd rather throw errors at broken userland handlers than ignore errors. Since when is this ignored?

I think it's been ignored all the time.

narfbg · 2014-03-19T00:36:28Z

I've mentioned this previously on internals and this probably should've been in a separate patch, but it's not, so ... I'd suggest createId(), validateId() instead of createSid(), validateSid().

It just makes more sense with a fully-qualified name is SessionHandler::createId() (and if there's procedural style, take session_id() for example - it's not called session_sid()).

yohgaki · 2014-03-19T00:43:27Z

Making createSid() into this patch is much easier. Consider working on the same code that is actively changing. It's not simple task to do.

Name could be changed. "SID" is used It's the way it is now. However, SID is used elsewhere like session ID constant.
http://jp1.php.net/manual/en/session.constants.php

I'm not sure which one would be better.

narfbg · 2014-03-19T00:50:14Z

Making createSid() into this patch is much easier. Consider working on the same code that is actively changing. It's not simple task to do.

Yea, I'm aware of that ... I'd give some reasoning for separating them, but probably another time.

Name could be changed. "SID" is used It's the way it is now. However, SID is used elsewhere like session ID constant.

Well, the SID constant is a different animal ... it's not in a class, just 'ID' would be stupid and somebody just chose to prefix it with S instead of i.e. 'SESSION_'. Class members have a context, it doesn't make sense to prefix them in such a way.

narfbg · 2014-03-19T01:23:17Z

ext/session/tests/session_set_save_handler_class_001.phpt

+		// cannot call parent as follows.
+		// return parent::validateSid($key);
+	}
+	public function updateTimestamp($key, $data) {


Why does this accept data? Why can't a user call parent::updateTimestamp($key)?

Underlying system may not allow to update time stamp. In this case, it would be redirected to write(). That's the reason why.

Well, can't you just ignore updateTimestamp() declarations internally if session.save_handler doesn't support them? As is, this is very inconvenient and unintuitive.

Plus, I assume this is only for PECL extensions. It wouldn't be the first case where some pecl extension needs an update to support a newer PHP version - can't we just mail them or something?

If I include this to SessionHandler object, it breaks existing save handlers hard. It tries to call "base" methods of new API which would be bogus calls for almost all save handlers.

I don't get it ... why would those calls be bogus?

You need understanding of current object based save handler implementation. Since it's not documented at all in the manual, it's hard to read it from code.

Anyway, in short, if open()/etc is implemented by user, it doesn't open base save handler. If user doesn't open base save handler, then calling validateSid()/updateTimestamp() is invalid, since base save handler is not opening session storage.

I understand that, but it's the same with every other method ... isn't it?

parent::write() also wouldn't have access to a file descriptor if open() was overloaded without calling it's parent. Or would it? If it's possible for write(), it must also be possible for updateTimestamp().

If user didn't open base handler, it does not work at all. Current users write handler for old API and do not have new API defined. It results in error always for existing save handler.

Object based save handler does some hack.
It extends "previously registered" save handler. Previous save handler functions are registered as it's default save handler. i.e.

ini_set('session.save_handler', 'files);
$handler = new SessionHandler;

just creates files save handler if user didn't extend it. If user extend open(), it does not open file save handler and other method calls become invalid unless user extend them all. See relevant tests for details.

yohgaki · 2014-03-19T06:17:42Z

Well, the SID constant is a different animal ... it's not in a class, just 'ID' would be stupid and
somebody just chose to prefix it with S instead of i.e. 'SESSION_'. Class members have a context, it
doesn't make sense to prefix them in such a way.

I don't care much about SID or ID, it's been named like it since there is create_sid() method mostly.
There may be other than the constant/method. SID is used as SessionID elsewhere in the module.

…ult for testing purpose. ToDo: Check tests if they are OK. Missing tests, etc.

narfbg · 2014-03-19T11:29:20Z

I don't think you're getting my point ... SID is named (and IMO, shouldn't have been) as a shorthand for SessionID. When you call SessionHandler::createId(), you already know that you're creating a session ID.

yohgaki · 2014-03-19T19:58:56Z

I see your point.
Could you send mail to internal?

narfbg · 2014-03-19T20:38:20Z

I suggested that on internals at least twice and was ignored ... I guess it gets lost in between all the arguments. This isn't something new, as I previously said: we already have session_id(), session_regenerate_id() - they don't have 'sid' for a reason.

Switching away from create_sid() was your idea in the first place, so why not just say 'createId' instead of 'createSid' from now on ... that should help. :)

Edit: Oh, and the same goes for validateSid() -> validateId().

yohgaki · 2014-03-19T20:41:53Z

I'm OK with ether name. I just not sure others. I'll rename it to see if anyone have comment.

yohgaki · 2014-03-19T20:51:03Z

@narfbg Test could be improved a lot. Could you contribute? Just fork this and send me PR.

narfbg · 2014-03-19T21:00:01Z

Sure, I'd just prefer to wait until we sort out the final solution.

Conflicts: ext/session/mod_files.c ext/session/session.c

Majkl578 · 2014-07-06T02:33:11Z

Ping @yohgaki + @Tyrael, if I see correctly, this RFC was accepted for 5.6 (read only, lazy write option), but hasn't been merged yet, why?

Tyrael · 2014-07-06T07:16:35Z

I'm not sure, there were some post-vote discussion about the session related changes suggested by @yohgaki, and maybe that is the reason that when @jpauli asked why this still not merged he didn't got a reply:
http://grokbase.com/p/php/php-internals/143cdp4c7w/php-dev-revert-session-serializer-name-session-gc
As I mentioned previously on the mailing list: any feature targeting 5.6 should be accepted and merged before the first beta, so I think this rfc missed 5.6 and should be merged into master instead.

yohgaki · 2015-01-25T06:42:28Z

New patch for master is this.
#1016
Closing this PR

Yasuo Ohgaki added 13 commits January 30, 2014 10:40

Merge remote-tracking branch 'upstream/PHP-5.4' into PHP-5.6

01957a8

Merge remote-tracking branch 'upstream/PHP-5.6' into PHP-5.6

e8007de

rfc-session-lock

7fdb14c

Check val before decode

65a693f

Fix test description

12fcf81

Rename lock name to "unsafe_lock"

4883b98

Remove "unsafe_lock", "lazy_destroy"

9cfa298

Remove unneeded and clean up a little

472798b

More clean up

bd8dbca

Initial INI handling with session_start(). More clean ups

3b140d5

Use of php_session_abort() is the fastest

6f99c0f

Revert "Use of php_session_abort() is the fastest"

235fd2a

I need new internal API for this. I'll write one later. This reverts commit 6f99c0f.

smalyshev reviewed Mar 18, 2014
View reviewed changes

Yasuo Ohgaki added 2 commits March 18, 2014 18:50

WIP: Renamed interface, made SessionUpdateTimestampHandler object. Ma…

87a792b

…de read_only acutually a read_only. ToDo: Bring back "lazy_write" option. Fix tests for new SessionUpdateTimestampHandler. Do more tests, especially for object based user save handlers.

WIP: Remove SessionUpdateTimestampHandler object.

37eb9a5

ToDo: Bring back lazy_write option. Fix tests.

narfbg reviewed Mar 19, 2014
View reviewed changes

Yasuo Ohgaki added 2 commits March 19, 2014 15:03

Update source code comments

b581755

Fix typo

28a76dc

WIP: Bring back "lazy_write" option as INI option. It enabled by defa…

297898e

…ult for testing purpose. ToDo: Check tests if they are OK. Missing tests, etc.

Rename createSid -> createId, validateSid -> validateId

c15c1fe

Yasuo Ohgaki added 4 commits April 1, 2014 08:36

Rename "read_only" option to "read_and_close"

9ba96e6

Merge branch 'PHP-5.6' into PHP-5.6-rfc-session-lock

7d45120

Conflicts: ext/session/mod_files.c ext/session/session.c

See if travis failure is fixed

b8b4a3c

Fix travis failure

cf424e8

smalyshev added the RFC label Nov 18, 2014

yohgaki closed this Jan 25, 2015

PHP 5.6 rfc session lock #628

PHP 5.6 rfc session lock #628

Uh oh!

Conversation

yohgaki commented Mar 18, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narfbg commented Mar 19, 2014

Uh oh!

yohgaki commented Mar 19, 2014

Uh oh!

narfbg commented Mar 19, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yohgaki commented Mar 19, 2014

Uh oh!

narfbg commented Mar 19, 2014

Uh oh!

yohgaki commented Mar 19, 2014

Uh oh!

narfbg commented Mar 19, 2014

Uh oh!

yohgaki commented Mar 19, 2014

Uh oh!

yohgaki commented Mar 19, 2014

Uh oh!

narfbg commented Mar 19, 2014

Uh oh!

Majkl578 commented Jul 6, 2014

Uh oh!

Tyrael commented Jul 6, 2014

Uh oh!

yohgaki commented Jan 25, 2015

Uh oh!

Uh oh!