[drupal-devel] [bug] race condition in variable_set in bootstrap.inc

[Jeremy]

Issue status update for http://drupal.org/node/20114

 Project:      Drupal
 Version:      cvs
 Component:    base system
 Category:     bug reports
 Priority:     critical
 Assigned to:  Ricetons
 Reported by:  Ricetons
 Updated by:   Jeremy at kerneltrap.org
 Status:       patch

What keeps another session from doing a DELETE after your session does
its SELECT COUNT but before it does its UPDATE?
Or, what keeps another session from doing an INSERT after your session
does its SELECT COUNT but before it does its INSERT?
All you've done is add another query...
I think you'll need to use locking to fix this right.


Jeremy at kerneltrap.org



Previous comments:
------------------------------------------------------------------------

April 6, 2005 - 22:53 : Ricetons

This bug was exposed by the poormancron module.
We can see that the module itself contains no bug. But in the function
variable_get(),
the function just execute SQL like this:
DELETE bar WHERE key="foo"
INSERT INTO bar VALUES(foo, bar2ooo)
but suppose this:
thread A and thread B has executed the DELETE sentence. And they do
INSERT at the same time.
then an SQL error like this occured:
duplicated key 'foo' in table "variables"
It's a critical bug. It should be fixed before the time that drupal
4.6.0 released.


------------------------------------------------------------------------

April 8, 2005 - 03:01 : robertDouglass

I also experience this on a site that has many different cron cycles
ranging from 2 minutes to 6 hours. The query errors which arise (as
noted above) are impossible to reproduce through normal testing yet
occur fairly (surprisingly) often in the logs. This doesn't just apply
to cron jobs but rather to all code that calls variable_set. Setting to
CVS since this hasn't been addressed since the issue was opened.


------------------------------------------------------------------------

April 8, 2005 - 08:10 : jhriggs

Attachment: http://drupal.org/files/issues/variable_set_update.patch (922 bytes)

I'm not sure that there is a "right" way to fix this.  In mysql, we
could use a REPLACE rather than DELETE/INSERT, but that wouldn't work
for pgsql.  And, if we are really seeing this type of race, there is no
telling which REPLACE would actually make it last and be saved.
Instead, we could do a check to see if the row exists and do an UPDATE
if it does and an INSERT otherwise;  however, this would be prone to
the same problem if we are seeing such a race.  That is, they may both
see that the row does not exist and both attempt to do an INSERT.  This
will only happen the first time a variable is created, though, which may
be more acceptable than what we are seeing now.  If the row does already
exist, they would both do UPDATES and would "succeed" in the sense that
there is no duplicate key error.  There is still no telling which one
actually sticks, though.
The attached patch takes the second approach.