Skip to content

CakePHP generated UPDATE query with WHERE 1 = 1 #2382

Closed
yevy opened this Issue Nov 22, 2013 · 35 comments

9 participants

@yevy
yevy commented Nov 22, 2013

My CakePHP application unexpectedly updated all rows of a table, setting one column to the same value. Using MySQL bin logs, I tracked down the culprit to the following query:

UPDATE `database`.`table` SET `column` = 12345 WHERE 1 = 1

I have not been able to figure out how my model operations could have resulted in that query being generated. There are only a few pieces of code that update that column, and they are executed many thousands of times a day without issue.

My guess is the following code may be to blame:

$this->MyModel->id = $id
$this->MyModel->saveField('column' , $value);

If a race condition occurs where the row the ID is set is deleted before saveField executes, could the offending query end up being generated? My tests resulted in a new row being created instead, but that's my best guess at the moment.

Has anyone encountered something like this with Cake?

I am using CakePHP version 2.1.1

This is reminiscent of a bug we encountered with an older version of Cake where a DELETE statement was generated with condition WHERE 1 = 1. That bug was eventually patched - http://cakephp.1045679.n5.nabble.com/model-gt-delete-error-td2739174.html. However this is the first time I'm seeing a similar issue with an UPDATE statement.

This issue was also posted to StackOverflow: http://stackoverflow.com/questions/20135980/cakephp-generated-update-query-with-where-1-1

@jippi
CakePHP member
jippi commented Nov 22, 2013

Hi @yevy

Can you please try and update to the latest 2.x version?

And paste your related models and their callbacks - since out of the box, the code should work just fine as provided above.

The ideal "proof" would be to provide a unit test to show case the problem. If you do not know how to create a test case, please gist the relevant code so we can see it :)

@yevy
yevy commented Nov 22, 2013

Hi @jippi

Thank you for your response.

This issue has surfaced in the production environment of a large application, so updating to the latest Cake version isn't going to be possible in the immediate future.

For better or worse, this issue isn't something I can readily reproduce, or confidently narrow down to a particular piece of code, which is why I shared generic statements I suspect generated the destructive SQL, rather than actual code.

I am hoping that someone familiar with Cake internals has seen this sort of SQL being generated and can postulate on the conditions that would cause it to happen.

@ADmad
CakePHP member
ADmad commented Nov 23, 2013

@yevy any chance of an Model::updateAll() being involved?

@jippi
CakePHP member
jippi commented Nov 23, 2013

Model::updateAll() do seem like the most plausible explanation.. considering the severity of the potential bug if it indeed is saveField that caused the query - then I'm really surprised it haven't shown up before.

Without a clean path to reproduce it, I think it's safe to say it's a bug in the application - that some 3rd party code has caused it rather than the suggested root cause suggested by @yevy

The cake test suite, and a multitude of applications uses this method, and has been using it for years, without this ever been raised.

There may some symptom in common for those that apparently have seen this error, but without some code to reproduce it, or even a description on how to reproduce it, cleaned of application specific code - I would just close it as invalid

@tommycows

Wow, this is a concindence! For some time, we've been experiencing a similar issue - many, many updates, all working fine, but at some point a WHERE 1 = 1 statements slips in there.

The first time, I failed to replicate, and somewhat unfairly thought that the client may have done something in phpMyAdmin.

The second time, I was confused, and turned on intense logging all over the place to try and track it down.

The third time is this morning, many many months later.

I agree with jippi that you'd expect an error like this to be in the client code. However, by the same logic, I don't understand how such a well-used application such as ours isn't running into this issue more often.

In the SQL log, the update is surrounded by a couple of abnormal statements:

DELETE ModelName FROM database.table_name AS ModelName WHERE ModelName.id = 91781
UPDATE database.table_name SET affected_column = '0' WHERE 1 = 1
INSERT INTO database.table_name (different_column, created) VALUES ('1', '2013-11-22 13:23:13')

I know it's a long short, but does the above sequence indicate anything?

Edit: this is with Cake v 2.1.3

@majna
majna commented Nov 23, 2013

Could this be an issue with DboSource::cacheMethod() ?

Caches result from query parsing operations. Cached results for both DboSource::name() and
DboSource::conditions() will be stored here. Method caching uses `md5()`. If you have
problems with collisions, set DboSource::$cacheMethods to false.

https://github.com/cakephp/cakephp/blob/2.5/lib/Cake/Model/Datasource/DboSource.php#L54

I also had an issue with growing _cake_core_method_cache in APC because SQL segments like this are cached by the same method:

'883a0365efc8c5fb947c6882e982524a' =>  array (
      1 => '(SELECT ... WHERE `Bid`.`user_id` = 2454654' ....

These segments are coming from Model::find() subquery:

'fields' => array(
    "(SELECT ... AS `Bid` WHERE `Bid`.`user_id` = $userId)",
    'User.id'...

In this example user_id is changing constantly (as some other fields) and eventually cache got pretty large (as possibility for md5 collisions), so I've turn it off. I've ran some benchmarks and haven't noticed decrease in performance. I suggest you guys turn this off.

@majna
majna commented Nov 23, 2013

Considering it happens rarely and abnormal statements in query are reported, I think my point could be valid.

@ceeram
CakePHP member
@ADmad
CakePHP member
ADmad commented Nov 23, 2013

@ceeram, @yevy mentioned that he is using 2.1.1 and @tommycows 2.1.3

@majna
majna commented Nov 23, 2013

Jelle replaced crc32 with md5 in 2.1.4
18b335a

Anyway, md5 collision is very rare, but if there's a lot of already hashed keys (like in method cache) it could happen a bit often.

@ADmad
CakePHP member
ADmad commented Nov 23, 2013

My second guess would be counter cache and third would be habtm :)

@majna
majna commented Nov 23, 2013

I agree with @ADmad

crc32/md5 collision could create "abnormal" SQL, but not WHERE 1 = 1 condition

@ADmad
CakePHP member
ADmad commented Nov 23, 2013

What type of fields are being updated with those queries with WHERE 1 = 1? Are they foreign key fields?

@markstory
CakePHP member

Crc32 collisions are not super hard to make happen. I would try turning the method cache off as well, at least until you can get to a newer version that does not use crc32.

@yevy
yevy commented Nov 23, 2013

@tommycows: isn't it comforting to know that someone else is struggling with a similar hard to track down issue?

@ADmad: there are no updateAll() or saveAll() statements in the code. It is not a counter or a foreign key field. This field is always updated by itself, for a single record, via saveField. I understand it would be theoretically possible to pass a data structure to a save() that could affect only that field, but I just don't see where that could be happening.

@majna, @markstory: I will turn off cacheMethods as a precaution, and will look to upgrade Cake as soon as it is feasible.

Thank you all for your insights.

I am wondering if it would be possible to isolate where Cake generates its UPDATE queries, and put a trap there for a "WHERE 1 = 1" condition. Otherwise I can't help but feel that our production servers are a data integrity disaster waiting to happen.

@markstory
CakePHP member

You should be able to trap the query generation in DboSource::renderStatement() or DboSource::buildStatement().

@tommycows

Hi everyone, sorry for taking so long to reply!

@yevy Hehe, whilst it is comforting to know I'm not all alone, I'm also feeling incredibly sad that you're in the same boat as me! :-)

@ADmad - no foreign keys being updated, just boolean (well, TINYINT...) flags.

I've just taken a look at the model cache mentioned, and for reasons I don't fully understand (maybe hot-patched by a previous dev?), the CRC32() call has already been replaced with MD5().

I've just run a grep countercache * -ir on the app directory codebase and come up with nothing. I'm under the impression it's something you have to enable; is that correct?

@yevy
yevy commented Nov 26, 2013

@tommycows: yes, counter cache is a model relationship that has to be explicitly specified. And I appreciate the sympathy!

@markstory: method cache collisions sound like a possibility, especially with the pred-MD5 version of Cake that we are running. But as mentioned above, that would seem more likely to cause an incorrect previously cached query to be run, rather than cause a WHERE 1 = 1 condition to show up. For now, I am leaving cacheMethods = true and trapping WHERE 1 = 1 conditions in Mysql->update(). It's a heavy-handed approach, but protecting the data is a priority until we can isolate the cause of this issue.

@lorenzo
CakePHP member
lorenzo commented Jan 22, 2014

Closing this as there has been no feedback for a couple months

@lorenzo lorenzo closed this Jan 22, 2014
@lorenzo lorenzo removed the On hold label Sep 19, 2014
@msemelman

Sorry to wake up this, but today something really similar to this happened to me.
saveField affected all records in table. btw, this made us lose lots of money today.
Using Cake version 2.5.0.
Further down here you can see the stacktrace as shown by newrelic. The savefield method should have update just one record, instead it affected 1100000 user accounts. Making us send them many emails. =(
The subscriptor model mentioned along the code uses always the same id which is set at the beggining of the method.
I was not able to see the query stating 1=1 or something similar. But given the length of the query (103 seconds) and the result in the database (all records where assigned recibir_top_ten=1 when usually only 10% had 1) I would say that the same happen here.
cacheMethods is enabled and I think it's possible that a collission had occured.
It would be nice in the future to check how many records are being affected by the "saveField" method.
Or maybe doing a canonical construction of the query, i see many disparities for the same kind of query along the code. The update query uses a different version of condition (doesn't name the table with an alias).
One more thing you can notice is that the second saveField after the long query just fails. I think it somehow has lost the $id value of the Model.

Trace:

image
image
image
image

@msemelman

BTW, hello to @yevy and @tommycows, we could start a club.

@tommycows

Hey @msemelman :-)

The bad news is that I never tracked down the bug. The code base involved is now out of my hands. From memory though, I did a really nasty dirty hack to intercept all SQL queries, and if the query contained the offending 1=1 bit, log it and ignore it.

Again from memory, the log file showed that there was only 1 bad query, so we were safe to filter it out. Just a damned shame I didn't have the time to trace and figure out what was going wrong behind the scenes!

@lorenzo
CakePHP member
lorenzo commented May 8, 2015

@msemelman what version of cake are you using?

@msemelman

@tommycows No problem, thank you anyway.

@lorenzo 2.5.0

@lorenzo
CakePHP member
lorenzo commented May 8, 2015

I remember having this problem in 2.4.10 but it never happened again after migrating to 2.5.1

@msemelman

@lorenzo according to http://cakephp.org/changelogs/2.5.1 nothing related to our issue has been touched.

@lorenzo
CakePHP member
lorenzo commented May 8, 2015

That's what I remember, sorry. It was a long time ago :P

@lorenzo
CakePHP member
lorenzo commented May 8, 2015

@msemelman It was actually 2.5.3 ace30fd

@msemelman

I can tell you that Subscriptor.id is 1341114 if it is useful somehow.
I don't know exactly how the md5 is applying to the condition. May be you can help me with that.

@yevy
yevy commented May 8, 2015
@msemelman

Oh, so stupid, i didn't check properly the other issue. Mine bad.

@msemelman

I think updating at least to 2.5.3 will solve this.

@msemelman

Thank you @lorenzo for your time!

@lorenzo
CakePHP member
lorenzo commented May 8, 2015

No problem! I would suggest updating to latest stable in the 2.x branch

@msemelman

Yes, we are on that asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.