Hi,
Trying to fix ticket #1485659.
There's a race condiction, in session handling. I think I got that with attached patch.
Session data is stored serialized(), but not exactly. So I had to create extra unserialize/serialize functions for that.
Downside is that unset($_SESSION['var']) will not work reliable anymore, so an extra rcube_sess_unset('var') was needed as well.
Please point me at any mistakes I made :)
Robin
--- 8< --- detachments --- 8< --- The following attachments have been detached and are available for viewing. http://detached.gigo.com/rc/xp/kFPRx0aF/_1485659.patch Only click these links if you trust the sender, as well as this message. --- 8< --- detachments --- 8< ---
List info: http://lists.roundcube.net/dev/
Robin Elfrink wrote:
Trying to fix ticket #1485659.
There's a race condiction, in session handling. I think I got that with attached patch.
your patch is not fixing race condition
Session data is stored serialized(), but not exactly. So I had to create extra unserialize/serialize functions for that.
But what's the real reason? Why PHP's auto-serialization isn't working? Maybe it's a problem with mysql's TEXT column size described in PHP manual? Change column type to MEDIUMTEXT (ALTER TABLE session MODIFY vars mediumtext NOT NULL) and try then.
A.L.E.C wrote:
There's a race condiction, in session handling. I think I got that with attached patch.
your patch is not fixing race condition
I think it does. This is what I observed (chronologically):
- getunread is called, reading session data
- I click on 'compose'
- compose session data is being read and written
- I compose an email, adding a recipient
- autocomplete is called, reading session data
- autocomplete's session data is written to databse
- getunread finishes, writing session data
At that moment 'compose' session data is gone.
What I did is merge existing session data with new session data, instead of overwriting.
Session data is stored serialized(), but not exactly. So I had to create extra unserialize/serialize functions for that.
But what's the real reason? Why PHP's auto-serialization isn't working? Maybe it's a problem with mysql's TEXT column size described in PHP manual? Change column type to MEDIUMTEXT (ALTER TABLE session MODIFY vars mediumtext NOT NULL) and try then.
php's unserialize() doesn't handle the 'name|serializeddata;othername|serializeddata;' structure.
Robin _______________________________________________ List info: http://lists.roundcube.net/dev/
Robin Elfrink wrote:
your patch is not fixing race condition
I think it does. This is what I observed (chronologically): What I did is merge existing session data with new session data, instead of overwriting.
My mistake, I've read the patch more precise now. I think it could fix some issues. One thing, rcube_sess_unset() and rcube_sess_write() are not atomic (should we use SELECT FOR UPDATE?).
php's unserialize() doesn't handle the 'name|serializeddata;othername|serializeddata;' structure.
I see now, it's needed for data merging. session_real_decode() from comments to http://php.net/manual/en/function.session-decode.php should be better.
On Wed, Apr 15, 2009 at 1:12 PM, A.L.E.C alec@alec.pl wrote:
Robin Elfrink wrote:
your patch is not fixing race condition
I think it does. This is what I observed (chronologically): What I did is merge existing session data with new session data, instead of overwriting.
My mistake, I've read the patch more precise now. I think it could fix some issues. One thing, rcube_sess_unset() and rcube_sess_write() are not atomic (should we use SELECT FOR UPDATE?).
php's unserialize() doesn't handle the 'name|serializeddata;othername|serializeddata;' structure.
I see now, it's needed for data merging. session_real_decode() from comments to http://php.net/manual/en/function.session-decode.php should be better.
+1 -- using that as well on another project. _______________________________________________ List info: http://lists.roundcube.net/dev/
On Wed, 15 Apr 2009 13:12:53 +0200, "A.L.E.C" alec@alec.pl wrote:
some issues. One thing, rcube_sess_unset() and rcube_sess_write() are not atomic (should we use SELECT FOR UPDATE?).
Is that MySQL-specific?
php's unserialize() doesn't handle the 'name|serializeddata;othername|serializeddata;' structure.
I see now, it's needed for data merging. session_real_decode() from comments to http://php.net/manual/en/function.session-decode.php should be better.
That looks more solid than my simple preg_split(). Used that in attached patch.
Any other comments? I'd like to submit this; it will make some people very happy :) We can handle the non-atomicness later.
Robin
--- 8< --- detachments --- 8< --- The following attachments have been detached and are available for viewing. http://detached.gigo.com/rc/AT/exvuhi5l/_1485659.20090415152.patch Only click these links if you trust the sender, as well as this message. --- 8< --- detachments --- 8< ---
List info: http://lists.roundcube.net/dev/
On Wed, Apr 15, 2009 at 3:33 PM, Robin Elfrink robin@15augustus.nl wrote:
On Wed, 15 Apr 2009 13:12:53 +0200, "A.L.E.C" alec@alec.pl wrote:
some issues. One thing, rcube_sess_unset() and rcube_sess_write() are not atomic (should we use SELECT FOR UPDATE?).
Is that MySQL-specific?
I don't think. Postgres and Oracle do that (as well).
php's unserialize() doesn't handle the 'name|serializeddata;othername|serializeddata;' structure.
I see now, it's needed for data merging. session_real_decode() from comments to http://php.net/manual/en/function.session-decode.php should be better.
That looks more solid than my simple preg_split(). Used that in attached patch.
Any other comments? I'd like to submit this; it will make some people very happy :) We can handle the non-atomicness later.
Robin
--- 8< --- detachments --- 8< --- The following attachments have been detached and are available for viewing. http://detached.gigo.com/rc/AT/exvuhi5l/_1485659.20090415152.patch Only click these links if you trust the sender, as well as this message. --- 8< --- detachments --- 8< ---
List info: http://lists.roundcube.net/dev/
List info: http://lists.roundcube.net/dev/
On Wed, 15 Apr 2009 15:39:03 +0200, till klimpong@gmail.com wrote:
some issues. One thing, rcube_sess_unset() and rcube_sess_write() are not atomic (should we use SELECT FOR UPDATE?).
Is that MySQL-specific?
I don't think. Postgres and Oracle do that (as well).
As far as I can see 'SELECT ... FOR UPDATE' is an SQL92 standard, so I added that.
Also, rcube_sess_unset did unset, but the rcube_sess_write in it undid the unset again :) Fixed that.
Robin
--- 8< --- detachments --- 8< --- The following attachments have been detached and are available for viewing. http://detached.gigo.com/rc/Jt/6IqQQ4kn/_1485659.20090415161.patch Only click these links if you trust the sender, as well as this message. --- 8< --- detachments --- 8< ---
List info: http://lists.roundcube.net/dev/
Robin Elfrink wrote:
As far as I can see 'SELECT ... FOR UPDATE' is an SQL92 standard, so I added that.
As I know it's not supported by mssql and sqlite, so we need probably a check for db type.
A.L.E.C wrote:
As far as I can see 'SELECT ... FOR UPDATE' is an SQL92 standard, so I added that.
As I know it's not supported by mssql and sqlite, so we need probably a check for db type.
We could modify the session table, or create an extra session_vars table, to hold 'key=>value' pairs. Then remember what we got with rcube_sess_read, do a diff at rcube_sess_write, and insert/update/delete only the keys that were changed in that process.
The only possible race left, as far as I see now, is when two concurrent processes modify the same key variables.
I'd like submit the patch I made but without the 'FOR UPDATE', so we can close #1485659 and make some people happy. We can have some more thoughts about the above later. OK?
Robin
List info: http://lists.roundcube.net/dev/
Hi RC Devs,
My logs/errors are full of same error. Can it be a bug? (I'm using the lastest trunk version)
PHP Error: Error loading template for in /var/www/html/roundcubemail-0.2-stable-trunk-r2419/program/include/rcube_template.php on line 337 (POST /webmail/?_task=mail&_action=autocomplete?_task=&_action=)
-- Victor Benincasa http://netbit.com.br
List info: http://lists.roundcube.net/dev/
-
A.L.E.C -
Robin Elfrink -
till -
Victor Benincasa