- Development - lists.drupal.org

[drupal-devel] [bug] race condition in variable_set in bootstrap.inc
by Ricetons 09 Apr '05

09 Apr '05

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: Ricetons Status: patch variable_set() is a function being used almost everywhere in drupal. INSERT/DELETE caused a serious performance problem. Maybe we should come to a soultion which uses a miminum number of queries. In my opinion, it seems that the third patch can solve the problem, but it's not good enough. By the way, table locking costs too much. I don't agree to use that method to solve this case. Ricetons Previous comments: ------------------------------------------------------------------------ April 7, 2005 - 11: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 - 16: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 - 21: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. ------------------------------------------------------------------------ April 8, 2005 - 21:37 : Jeremy(a)kerneltrap.org 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. ------------------------------------------------------------------------ April 8, 2005 - 22:46 : jhriggs 1. Assuming that all we are talking about is variable_set() calls, DELETE is not an issue, because the patch removes deletes. It only does updates and inserts. Now if you throw in variable_del() calls, that's another story, but it's also outside the scope of this issue. It would likely be much more rare than the delete/insert problems we see now. 2. Your second example is the only lingering downfall. As I mentioned in my post: "...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." 3. This doesn't add any queries. Right now it does DELETE and INSERT. With the patch it does SELECT and UPDATE/INSERT. Two queries either way. As I alluded, this may not be a true fix -- I don't think there is one for race conditions like this, even with table-locking -- but it addresses the problem in a way that makes sense and eliminates an error in the majority of cases. ------------------------------------------------------------------------ April 8, 2005 - 23:42 : killes(a)www.drop.org @Jeremy: db locking will become available with the revisios patch. I don't advise to use it too mch because it is very slow... I actually do not see why this needs a fix. All it is going to produce is a message in the error log... ------------------------------------------------------------------------ April 9, 2005 - 06:06 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/variable_set.patch (708 bytes) The attached patch makes more sense to me. The same method is used by cache_set() and statistics_exit(). Once a variable already exists in the variable table, we only need to do one db_query to update it...

1 0

[drupal-devel] [bug] Blocks with no title kill Drupal
by Morbus Iff 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/19965 Project: Drupal Version: cvs Component: block.module Category: bug reports Priority: normal Assigned to: Robin Monks Reported by: budda Updated by: Morbus Iff Status: patch I'm, personally, fine with how you've fixed the blank block title. You've really solved two problems in one: duplicate titles (which is the cause of this Issue), and the desire to have blank (or no) titles (which is the reason for this Issue) for the end user. With that said, I'm not a fan of your error message (but, then again, anal wording is my specialty, so don't think I'm picking on you): I'd rename it to "That block title is already in use; please choose another." The goals here: The user may not remember all the previous titles he's used. Using "unique" starts forcing him to remember. We shouldn't do that. The user already knows he entered it - we don't have to repeat that to him. It's active, not passive (no use of "have"). Use a semi-colon, not a comma. It's much shorter and concise. Morbus Iff Previous comments: ------------------------------------------------------------------------ April 4, 2005 - 12:03 : budda When setting more than one block's title to '' (blank) Drupal dies, and doesn't catch the SQL query error nicely. The error is as follows: user error: Duplicate entry '' for key 2 query: UPDATE boxes SET title = '', body = ' ...<cut>... in /var/www/html/drupal-cvs/includes/database.mysql.inc on line 66. Using 4.6 build from 4/4/2005 The page ends with: warning: Cannot modify header information - headers already sent by (output started at /var/www/html/drupal-cvs/includes/common.inc:384) in /var/www/html/drupal-cvs/includes/common.inc on line 192. ------------------------------------------------------------------------ April 8, 2005 - 03:08 : robertDouglass There should be a check before the insert is attempted and if the block title already exists the user should be notified with form_error(). ------------------------------------------------------------------------ April 8, 2005 - 04:46 : Poolio this certainly doesn't solve the problem, but I've worked around this issue by using html tags in the title field for blocks that I don't want to show a title. ------------------------------------------------------------------------ April 8, 2005 - 19:31 : Robin Monks Attachment: http://drupal.org/files/issues/block.module.blank.title.patch (955 bytes) A small patch that checks to ensure the title does not already exit in the Blocks list. Also, if the field was left blank, in adds an HTML comment with the current date to the second to ensure that a duplicate title isn't created. This is my first patch, so I probably did something wrong, but I'm willing to fix it too. Robin Monks MozNetwork.org

1 0

[drupal-devel] [bug] Blocks with no title kill Drupal
by Robin Monks 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/19965 Project: Drupal Version: cvs Component: block.module Category: bug reports Priority: normal -Assigned to: Anonymous +Assigned to: Robin Monks Reported by: budda Updated by: Robin Monks -Status: active +Status: patch Attachment: http://drupal.org/files/issues/block.module.blank.title.patch (955 bytes) A small patch that checks to ensure the title does not already exit in the Blocks list. Also, if the field was left blank, in adds an HTML comment with the current date to the second to ensure that a duplicate title isn't created. This is my first patch, so I probably did something wrong, but I'm willing to fix it too. Robin Monks MozNetwork.org Robin Monks Previous comments: ------------------------------------------------------------------------ April 4, 2005 - 17:03 : budda When setting more than one block's title to '' (blank) Drupal dies, and doesn't catch the SQL query error nicely. The error is as follows: user error: Duplicate entry '' for key 2 query: UPDATE boxes SET title = '', body = ' ...<cut>... in /var/www/html/drupal-cvs/includes/database.mysql.inc on line 66. Using 4.6 build from 4/4/2005 The page ends with: warning: Cannot modify header information - headers already sent by (output started at /var/www/html/drupal-cvs/includes/common.inc:384) in /var/www/html/drupal-cvs/includes/common.inc on line 192. ------------------------------------------------------------------------ April 8, 2005 - 08:08 : robertDouglass There should be a check before the insert is attempted and if the block title already exists the user should be notified with form_error(). ------------------------------------------------------------------------ April 8, 2005 - 09:46 : Poolio this certainly doesn't solve the problem, but I've worked around this issue by using html tags in the title field for blocks that I don't want to show a title.

1 0

[drupal-devel] [bug] make sure links are an array
by danielc 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/18835 Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Olen Updated by: danielc Status: patch Uwe makes a very good point. danielc Previous comments: ------------------------------------------------------------------------ March 13, 2005 - 08:55 : Olen Had a bug in some module where theme_links was called but '$links' was not an array. Ofcourse the real bug was in the module, but I theme_links should maybe not produce ugly errors if this happens? --- theme.inc.orig 2005-03-13 14:51:45.829133700 +0100 +++ theme.inc 2005-03-13 14:46:55.000000000 +0100 @@ -432,7 +432,9 @@ * A string containing the themed links. */ function theme_links($links, $delimiter = ' | ') { - return implode($delimiter, $links); + if (is_array($links)) { + return implode($delimiter, $links); + } } /** ------------------------------------------------------------------------ April 8, 2005 - 03:16 : chx I do not think it is the task of core to correct the incorrect work of non-core modules. ------------------------------------------------------------------------ April 8, 2005 - 17:54 : Uwe Hermann IMHO core should be programmed defensively and robust, and not fail on errors which it could detect and react upon properly.

1 0

[drupal-devel] [bug] make sure links are an array
by Uwe Hermann 09 Apr '05

09 Apr '05

Issue status update for http://drupal.org/node/18835 Project: Drupal Version: cvs Component: theme system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Olen Updated by: Uwe Hermann -Status: won't fix +Status: patch IMHO core should be programmed defensively and robust, and not fail on errors which it could detect and react upon properly. Uwe Hermann Previous comments: ------------------------------------------------------------------------ March 13, 2005 - 15:55 : Olen Had a bug in some module where theme_links was called but '$links' was not an array. Ofcourse the real bug was in the module, but I theme_links should maybe not produce ugly errors if this happens? --- theme.inc.orig 2005-03-13 14:51:45.829133700 +0100 +++ theme.inc 2005-03-13 14:46:55.000000000 +0100 @@ -432,7 +432,9 @@ * A string containing the themed links. */ function theme_links($links, $delimiter = ' | ') { - return implode($delimiter, $links); + if (is_array($links)) { + return implode($delimiter, $links); + } } /** ------------------------------------------------------------------------ April 8, 2005 - 10:16 : chx I do not think it is the task of core to correct the incorrect work of non-core modules.

1 0

[drupal-devel] [bug] race condition in variable_set in bootstrap.inc
by Jeremy 09 Apr '05

09 Apr '05

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(a)kerneltrap.org Status: patch Attachment: http://drupal.org/files/issues/variable_set.patch (708 bytes) The attached patch makes more sense to me. The same method is used by cache_set() and statistics_exit(). Once a variable already exists in the variable table, we only need to do one db_query to update it... Jeremy(a)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. ------------------------------------------------------------------------ April 8, 2005 - 08:37 : Jeremy(a)kerneltrap.org 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. ------------------------------------------------------------------------ April 8, 2005 - 09:46 : jhriggs 1. Assuming that all we are talking about is variable_set() calls, DELETE is not an issue, because the patch removes deletes. It only does updates and inserts. Now if you throw in variable_del() calls, that's another story, but it's also outside the scope of this issue. It would likely be much more rare than the delete/insert problems we see now. 2. Your second example is the only lingering downfall. As I mentioned in my post: "...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." 3. This doesn't add any queries. Right now it does DELETE and INSERT. With the patch it does SELECT and UPDATE/INSERT. Two queries either way. As I alluded, this may not be a true fix -- I don't think there is one for race conditions like this, even with table-locking -- but it addresses the problem in a way that makes sense and eliminates an error in the majority of cases. ------------------------------------------------------------------------ April 8, 2005 - 10:42 : killes(a)www.drop.org @Jeremy: db locking will become available with the revisios patch. I don't advise to use it too mch because it is very slow... I actually do not see why this needs a fix. All it is going to produce is a message in the error log...

1 0

[drupal-devel] [bug] On q=admin/settings "cache documentation" links to q=admin/help instead of q=admin/help/system#cache
by ankur 08 Apr '05

08 Apr '05

Issue status update for http://drupal.org/node/20197 Project: Drupal Version: cvs Component: system.module Category: bug reports Priority: minor Assigned to: Anonymous Reported by: ankur Updated by: ankur Status: patch Attachment: http://drupal.org/files/issues/system.module_1.diff (1.4 KB) I don't know if this is a bug, but on q=admin/settings, I followed the link labelled "cache documentation" in the form group labelled "Cache settings" and the link took me to q=admin/help where the word "cache" (or "caching") was nowhere to be found. Maybe it should link to "q=admin/help/system#cache"? -Ankur ankur

1 0

[drupal-devel] [task] MENU_ITEM_GROUPING items need better display
by drumm 08 Apr '05

08 Apr '05

Issue status update for http://drupal.org/node/20192 Project: Drupal Version: cvs Component: menu system Category: tasks Priority: normal Assigned to: Anonymous Reported by: drumm Updated by: drumm Status: patch Attachment: http://drupal.org/files/issues/menu.module.diff (731 bytes) Menu items with type MENU_ITEM_GROUPING are enver really disabled or enabled, but that is how the menu module's UI sees it now. There is a bit more to be done, but this is a good start (might be okay for 4.6). I think other enhancements for these special items will have to come with more sweeping UI changes to that module. drumm

1 0

[drupal-devel] [bug] User info should not be visible without permission
by killes 08 Apr '05

08 Apr '05

Issue status update for http://drupal.org/node/4166 Project: Drupal Version: cvs Component: user system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: mic Updated by: killes(a)www.drop.org Status: patch Attachment: http://drupal.org/files/issues/user_access_bug_3.patch (11.35 KB) Another iteration: Users should alwys be allowed to see their own user page. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ November 17, 2003 - 06:23 : mic When anonymous visitors do not have "Access userlist" permission, they can still view all the public info in user profiles. Drupal sites that are created for a group of friends or for an organization want to protect their e-mail addresses, telephone numbers and so on, while making these accessible to fellow members. This could be a critical feature request, but since I think it's an error, I'm sending you this as bug. (I don't have CVS, so I'm hoping someone else will make the simple correction needed to the user module) ------------------------------------------------------------------------ February 3, 2004 - 05:41 : daBrado Attachment: http://drupal.org/files/issues/user-module-add-view-permission.patch (1.54 KB) I made a patch that fixed this by adding a new permission, "access users". This is everything this patch does: Add new permission, "access users" If a user does not have the "access users" permission, s/he cannot read another user's profile at all, and instead gets an "access denied" page. I hope I did this in the proper way. ------------------------------------------------------------------------ April 21, 2004 - 20:25 : Dries I think we should not introduce a new permission but merge with the existing 'access user list' permission (or rename it to 'access users'). Marking this "won't fix" until the patch has been udpated. ------------------------------------------------------------------------ May 28, 2004 - 02:26 : daBrado Attachment: http://drupal.org/files/issues/user-module-add-view-permission_0.patch (1.88 KB) Another patch, this time renaming the permission "access user list" to "access users", and adding a check in the user viewing function to only allow users with this permission to view the user information. ------------------------------------------------------------------------ July 5, 2004 - 15:31 : Anonymous Attachment: http://drupal.org/files/issues/user-module-add-view-permission_1.patch (1.86 KB) Here is a patch again, now for CVS. Does the same thing as above. It is a very simple patch. If it seems proper, I hope that can be included before it goes stale. ------------------------------------------------------------------------ July 9, 2004 - 02:08 : daBrado Attachment: http://drupal.org/files/issues/user-module-add-view-permission_2.patch (1.33 KB) The previous patch was accepted, but then for some reason reversed as part of another CVS commit. So, here is a new patch that brings back the "access users" permission. It controls whether or not a user may view other users info. ------------------------------------------------------------------------ September 22, 2004 - 16:53 : Bèr Kessels Attachment: http://drupal.org/files/issues/access_users_perm.patch (6.46 KB) A new and revised patch. It adds an "access users" permission ------------------------------------------------------------------------ September 22, 2004 - 18:02 : killes(a)www.drop.org I'd like to see this patch applied to cvs before the 4.5 release. Now that we can protect our nodes from unauthorized access it just makes sense to protect our user data as well. In the future I'd like to see scheme where the user (as in end-user) is able to selet which of his data gets published. ------------------------------------------------------------------------ September 23, 2004 - 01:52 : rkendall I would like this too ------------------------------------------------------------------------ October 14, 2004 - 23:46 : drumm Can the name be changed to 'view user profiles'? ------------------------------------------------------------------------ October 18, 2004 - 09:19 : Bèr Kessels It can, but I used "access" for consistancy. we have "access newsfeeds", "access comments", "access etc". This patch is critical for corporate sites btw. A corporate site that has its customers information lying on the streets (so to say) is not-done. Ber ------------------------------------------------------------------------ November 27, 2004 - 13:49 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/user-access.patch (5.96 KB) Updated for CVS. I assumed that "admin users" implies "access users" in user_admin. ------------------------------------------------------------------------ March 1, 2005 - 17:30 : killes(a)www.drop.org Does not apply anymore, Ber can you have a look at it? ------------------------------------------------------------------------ March 7, 2005 - 09:19 : Bèr Kessels Attachment: http://drupal.org/files/issues/user_access_0.patch (9.74 KB) New patch. Should apply to HEAD. ------------------------------------------------------------------------ April 7, 2005 - 21:29 : killes(a)www.drop.org It's a patch (which still applies and wants into core). ------------------------------------------------------------------------ April 8, 2005 - 18:10 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/user_access_bug_1.patch (11.45 KB) Ok, updated patch. Includes changes to format_name to only display unlinked username for non-privileddged users. 'acccess users' is now 'access user profiles'. The search users tab won't get displayed either if you are not allowed to seee user profiles. The patch got even tested. ------------------------------------------------------------------------ April 8, 2005 - 18:19 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/user_access_bug_2.patch (11.26 KB) Further testing revealed a bug in the older parts of the patch...

1 0

[drupal-devel] [bug] race condition in variable_set in bootstrap.inc
by killes 08 Apr '05

08 Apr '05

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: killes(a)www.drop.org Status: patch @Jeremy: db locking will become available with the revisios patch. I don't advise to use it too mch because it is very slow... I actually do not see why this needs a fix. All it is going to produce is a message in the error log... killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ April 7, 2005 - 04: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 - 09: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 - 14: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. ------------------------------------------------------------------------ April 8, 2005 - 14:37 : Jeremy(a)kerneltrap.org 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. ------------------------------------------------------------------------ April 8, 2005 - 15:46 : jhriggs 1. Assuming that all we are talking about is variable_set() calls, DELETE is not an issue, because the patch removes deletes. It only does updates and inserts. Now if you throw in variable_del() calls, that's another story, but it's also outside the scope of this issue. It would likely be much more rare than the delete/insert problems we see now. 2. Your second example is the only lingering downfall. As I mentioned in my post: "...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." 3. This doesn't add any queries. Right now it does DELETE and INSERT. With the patch it does SELECT and UPDATE/INSERT. Two queries either way. As I alluded, this may not be a true fix -- I don't think there is one for race conditions like this, even with table-locking -- but it addresses the problem in a way that makes sense and eliminates an error in the majority of cases.

2 1