MyBB Community Forums

The limit portion of the query needs to be removed from the following locations. Limits should not be used when doing update/deletes since the row being removed will always be ambiguous. In addition, this causes all kinds of issues with binary logging/replication in statement mode in MySQL since the slave or cloned database could be out of sync if different rows are removed. In each case, there should never be duplicates for the where clause anyway.

Here is an example of the error produced

Quote:[Warning] Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT. The statement is unsafe because it uses a LIMIT clause. This is unsafe because the set of rows included cannot be predicted. Statement: UPDATE mybb_sessions

inc/class_moderation.php, lines 2643-2644

$db->update_query("threads", $new_subject, "tid='{$thread['tid']}'", 1);
$db->update_query("posts", $new_subject, "tid='{$thread['tid']}' AND replyto='0'", 1);

inc/class_session.php, line 416

$db->update_query("spiders", $updated_spider, "sid='{$spider_id}'", 1);

inc/class_session.php, line 461

$db->update_query("sessions", $onlinedata, "sid='{$sid}'", 1);

inc/functions.php, line 849

$db->update_query("sessions", $noperm_array, "sid='{$session->sid}'", 1);

inc/functions_archive.php, line 217

$db->update_query("sessions", $noperm_array, "sid='{$session->sid}'", 1);

inc/functions_user.php, all 5 of these queries...

104 - $db->update_query("users", $sql_array, "uid='".$user['uid']."'", 1);
113 - $db->update_query("users", $sql_array, "uid = ".$user['uid'], 1);
164 - $db->update_query("users", $newpassword, "uid='$uid'", 1);
217 - $db->update_query("users", $sql_array, "uid='{$uid}'", 1);
236 - $db->update_query("users", $sql_array, "uid='{$uid}'", 1);

inc/datahandlers/login.php, both of these queries...

181 - $db->update_query("users", $sql_array, "uid = '{$this->login_data['uid']}'", 1);
192 - $db->update_query("users", $sql_array, "uid = '{$this->login_data['uid']}'", 1);

inc/tasks/massmail.php, 2 queries

38 - $db->update_query("massemails", array('status' => 2), "mid='{$mass_email['mid']}'", 1);
146 - $db->update_query("massemails", $update_array, "mid='{$mass_email['mid']}'", 1);

admin/modules/forum/management.php, 2 queries...

1399 - $db->update_query("forums", array("parentlist" => make_parent_list($fid)), "fid='{$fid}'", 1);
1414 - $db->update_query("forums", array("parentlist" => make_parent_list($child['fid'])), "fid='{$child['fid']}'", 1);

admin/inc/functions.php, line 615

$db->update_query("adminoptions", array("loginlockoutexpiry" => TIME_NOW+((int)$mybb->settings['loginattemptstimeout']*60)), "uid='".(int)$uid."'", 1);

admin/inc/functions_themes.php, 3 queries

498 - $db->update_query("themestylesheets", array('cachefile' => $db->escape_string($stylesheet['name'])), "sid='{$stylesheet['sid']}'", 1);
511 - $db->update_query("themestylesheets", array('cachefile' => $db->escape_string($stylesheet['name'])), "sid='{$stylesheet['sid']}'", 1);
517 - $db->update_query("themestylesheets", array('lastmodified' => TIME_NOW), "sid='{$stylesheet['sid']}'", 1);

admin/index.php, 2 queries...

209 - $db->update_query("adminoptions", array("loginattempts" => 0, "loginlockoutexpiry" => 0), "uid='".(int)$mybb->user['uid']."'", 1);
272 - $db->update_query("adminoptions", array("loginattempts" => "loginattempts+1"), "uid='".(int)$login_user['uid']."'", 1, true);
283  - $db->update_query("adminoptions", array("loginlockoutexpiry" => TIME_NOW+((int)$mybb->settings['loginattemptstimeout']*60)), "uid='".(int)$login_user['uid']."'", 1);

So it is not recommended to use a LIMIT statement when the WHERE clause already takes care of that?

No because you never know which rows is going to be deleted.

Let's say your table is:
EMPLOYEE_ID, JOB_DESCRIPTION
Omar, Developer
Omar, Forum Moderator

And you run this query
delete from employee_table where employee_id='Omar' limit 1

Which row is going to be deleted? There is no way to tell.

Hi,

Thank you for your report. We have pushed this issue to our Github repository for further analysis where you can track our commits and progress with fixing this bug. Discussions regarding this bug may also take place there too.

Follow this link to visit the issue on Github: https://github.com/mybb/mybb/issues/1057

Thanks for contributing to MyBB!

Regards,
The MyBB Group

I think this should be only fixed for 1.8 but I see the last report of this was implemented to 1.6 as well so probably too, for consistency.

MyBB assumes rows to be unique for all of these. It's also dictated by the table structure, you'd get duplicate entry errors otherwise. So while the limit isn't required, there is no ambiguity either, with or without limit the result will always be the same. I guess the limit was used in the hopes it would help performance (which it doesn't).

I use the limit in my plugins hoping for a performance boost, though I know it is not required never knew it could cause issues.

(2014-07-26, 08:18 PM)frostschutz Wrote: [ -> ]MyBB assumes rows to be unique for all of these. It's also dictated by the table structure, you'd get duplicate entry errors otherwise. So while the limit isn't required, there is no ambiguity either, with or without limit the result will always be the same. I guess the limit was used in the hopes it would help performance (which it doesn't).

Tell that to my server where there is 5GB-worth of the message I quoted above in /var/log/mysqld.log

I agree though, that in this application there is technically no ambiguity, but it still should be removed.

In general, LIMIT shouldn't be used unless an unambiguous ORDER BY clause also exists. As previously mentioned, if there isn't an ORDER BY clause, there is no guarantee of which rows will be returned / deleted / updated.

(2014-07-26, 09:08 PM)spork985 Wrote: [ -> ]

(2014-07-26, 08:18 PM)frostschutz Wrote: [ -> ]MyBB assumes rows to be unique for all of these. It's also dictated by the table structure, you'd get duplicate entry errors otherwise. So while the limit isn't required, there is no ambiguity either, with or without limit the result will always be the same. I guess the limit was used in the hopes it would help performance (which it doesn't).

Tell that to my server where there is 5GB-worth of the message I quoted above in /var/log/mysqld.log

I agree though, that in this application there is technically no ambiguity, but it still should be removed.

I'll get that fixed now sorry for the waste of space!

Edit:
I've also edited this one: (class_session.php)

$query = $db->simple_select("spiders", "*", "sid='{$spider_id}'", array('limit' => 1));

And: (functions_user.php)

$query = $db->simple_select("users", "uid,username,password,salt,loginkey,usergroup", "uid='".(int)$uid."'", array('limit' => 1));

$query = $db->simple_select("users", "salt", "uid='$uid'", array('limit' => 1));