lock_acquire() does not properly extend expired lock [#1103310]

Comment	File	Size	Author
#9	lock_acquire-2.patch	697 bytes	c960657

#7	lock_acquire-1.patch	1.25 KB	c960657

	lock_acquire-D6.patch	1.25 KB	c960657

Comment #1

c960657 CreditAttribution: c960657 commented 10 May 2011 at 20:11

Assigned:

Unassigned

» c960657

Log in or register to post comments

Comment #2

drewish CreditAttribution: drewish commented 22 June 2011 at 21:38

Status:

Needs review

» Reviewed & tested by the community

Ran into this issue as well. The patch corrects the problem.

Log in or register to post comments

Comment #3

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 27 July 2011 at 07:58

Status:

Reviewed & tested by the community

» Needs review

Please provide a more detailed explanation of what is going on and more reviewers if possible. Thanks!

Log in or register to post comments

Comment #4

c960657 CreditAttribution: c960657 commented 28 July 2011 at 11:26

The problem is that the return value from db_query("UPDATE ... WHERE ...") is passed on to db_result() in order to see if any rows matched the WHERE clause of the query.

This is wrong. db_query("UPDATE ...") always returns TRUE, unless an error occurred (note that this is not described in the documentation of db_query()).

The proper way to see if any rows where matched by the WHERE clause of an UPDATE query is to use db_affected_rows(). This is what the patch does. If no rows where affected, it means that our lock is no longer in the database table and thus cannot be extended.

Log in or register to post comments

Comment #5

mgriego CreditAttribution: mgriego commented 4 August 2011 at 23:14

Seeing the same thing here when we try to extend an unexpired lock. As c960657 said, It is not proper to use db_result() on a db_query() return value when an UPDATE query is used. For MySQL systems, the result of mysql_query() where an UPDATE query is issued is simply TRUE or FALSE, not a resource as db_result() expects. FALSE only happens on error, and an UPDATE that doesn't update any rows is not an error, so the db_affected_rows() call in the supplied patch is the proper way of handling this.

From what I can tell, pg_query() returns a result resource regardless of the query type issued, so, if the locking implementation was initially written and tested against Postgres, then that may be why this wasn't caught before. We (and I'm sure most of the people using Drupal), however, use MySQL.