Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
function cache_clear_all($cid = NULL, $table = NULL, $wildcard = FALSE) {
and
db_query("DELETE FROM {". $table ."}");
When passing on the wrong value for $table the wrong table can be wiped out accidentally. This can easily occur when making a typo (like I just did) or when there's a bug in a third-party module that allows users to enter a value for $table indirectly (hardly plausible, but possible).
Comment | File | Size | Author |
---|---|---|---|
#31 | drupal-check-cache-bin-238250-31.patch | 3.6 KB | markpavlitski |
#23 | drupal-check-cache-bin-238250-23.patch | 2.07 KB | markpavlitski |
#18 | drupal-check-cache-bin-238250-18.patch | 1.79 KB | markpavlitski |
#9 | drupal-check-cache-bin-238250-9.patch | 1.6 KB | markpavlitski |
Comments
Comment #1
dpearcefl CreditAttribution: dpearcefl commentedDoes this issue exist in current D6?
Comment #2
dpearcefl CreditAttribution: dpearcefl commentedComment #3
pounardToday I tried:
and as a fun side effect it TRUNCAT'ed the 'block' table.
The fun part is that I did it on a production site. The other fun part is that all known cache bins are configured to use a MongoDB backend, but unknown ones still fallback on DrupalDatabaseCache backend per default.
The very fun part is that everything went very well and I got no errors.
Hopefuly, we have backups, it has been restored in 10 minutes, but think about it: any poorly coded module could use a variable as second cache_clear_all() parameter and potentially TRUNCATE *any* database table without further checks.
Just try with:
For additional fun.
What happens is that
cache_clear_all()
will load any cache object loosly and lazily without further checks, and call theclear()
method on it. TheDrupalDatabaseCache::clear()
method does not do any further security checks and will just run adb_truncate()
no questions asked.Escalating this to critical, this is a very dangerous behavior: cache_clear_all() MUST NOT arbitrary TRUNCATE any other table than a valid cache bin.
Drupal 8 handles cache very differently, and cache bins are supposed to be registered in the kernel (at least I hope) but Drupal 7 cache handling it way too lazy about this. At least some checks should be done before the TRUNCATE statement in order to ensure we are really hitting a cache table.
Comment #4
alexpottWhat's wrong with
drush cc
?Comment #5
pounardNothing is wrong with drush cc, that's actually not the point of this issue. The API method can do permanent damage to data where it shouldn't. This should be more robust.
Comment #6
pounardThere is a bunch of pathes to explore to fix this, I can see two obvious that would work:
- Force modules to register their bins with a hook, deny any cache operation on an unknow bin [breaks BC]
- Enforce the $bin parameter to start with 'cache_' by appending this prefix if not set in the cache_* helpers.
The second one was actually one of the first D8 improvements in the very begining even before SF adoption, I think is a good candidate for D7.
Comment #7
catchThere's nothing stopping a module registering a cache bin in Drupal 7 that doesn't start with cache_* so that'd also be a bc break.
Comment #8
pounard@catch In the latest 3 years, I never saw any module not using the 'cache_' prefix, except maybe one CTools table. Is there any other solution you could see?
Comment #9
markpavlitski CreditAttribution: markpavlitski commentedWe can use the database schema to confirm if a cache bin represents a genuine cache table.
This patch checks that all of the common cache table fields exist in the table before truncating.
I've only added a check for the truncate operation, since the missing fields will cause other cache operations to throw a PDO exception anyway.
Comment #11
pounardThis approach could actually work, I'm not comfortable with live database schema checks, but considering than the cache clear with no conditions is not supposed to happen often on a live site, why not. Let's see why it fails.
Comment #12
pounardThose are all PHP notices in the isValidBin() function.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedShooting your own foot doesn't qualify as a critical bug of the gun. Let's see how we can improve the situation for Drupal 8.
Comment #14
pounardThe status was here to get some attention, indeed it's not that critical, but it remains very dangerous.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commented@pounard: That's exactly why we have higher level tools. Alex is totally spot on in #4: that's why
drush cc
exists in the first place.Comment #16
pounardMy opinion is that only hard registration of bins can cleanly avoid that.
Today in 8.x nothing prevents you from fetching an arbitrary bin from Drupal\Core\Cache\CacheFactory::get() method, but the container hides the factory and will only provide you registered services such as "cache.binname", which are explictely registered via the Bundles or the *.services.yml file: in normal 8.x runtime (even in Drush if it uses the core correctly) reaching that code path can't happen if the API has been used the way it is designed for.
Switching back this issue as 7.x only because 7.x API allows this code path to be reached very easily, and is even the appropriate way of using the API.
Comment #17
pounard#15 is right, indeed, but in the end nothing prevents a module from passing a variable there, which at some point could have been dynamically built. Drush is not a Drupal core provided tool nor a requirement to be used, so it is definitely out of this issue scope.
Comment #18
markpavlitski CreditAttribution: markpavlitski commented@pounard This patch checks for cache and cache_* tables first, so only runs a schema check for edge cases. It should also fix the notices which caused #9 to fail.
Comment #19
pounardI do like the
if ($this->bin == 'cache' || substr($this->bin, 6) == 'cache_') {
first line! The schema check using the hook_schema() will populate the schema incremental cache, and might end up in a minor memory usage raise on the pages where the cache_clear_all() method would be triggered with those parameters, but I'm not really worried, it's not supposed to happen during a normal runtime. May I add I prefer this to straight db_field_exists() calls.I think this patch is good enough for core and will make this safer, the only problem it can cause is for poorly coded modules: those wont don't use the
cache_
prefix won't work anymore, but I'd say that:1) I don't think there are a lot of them out there
2) They need fixing, and patching them will be easy
In my opinion, for this kind of issue, security wins over compatibility, especially when it will explicitely break only modules that don't respect the core naming conventions.
Comment #20
markpavlitski CreditAttribution: markpavlitski commented@pounard It's worth noting that this will still work with modules using a non-standard table name, i.e.
{mymodule_cache}
, as long as they base their schema on the system cache schema. If not, their cache table probably won't work anyway.Comment #21
markpavlitski CreditAttribution: markpavlitski commented@pounard It's also worth noting that the potential memory increase only occurs in edge cases, i.e. when attempting to
cache_clear_all()
on an invalid bin or on a bin using a non-standard format.Comment #22
pounardIndeed, I spoke too quickly! I'm quite fond of this solution, it's straight-forward, induces aboslutely no performance penalty when using modules that respect the naming conventions, and fallbacks smartly when using modules violating the conventions, keeping BC.
I think this is RTBC, very nice, thanks!*
catch, Damien Tournoud, alexpott, I think we have a solid patch here, opinions?
EDIT: Just one additional note: I think we should document why this exists, anyone who read the code without any hint may find this useless and remove it. Maybe adding the issue link too.
Comment #23
markpavlitski CreditAttribution: markpavlitski commented@pounard Agreed. This is the patch from #18 with additional documentation referencing this issue page.
Comment #24
pounardThanks, I think that #23 is RTBC. EDIT: There's probably a few coding standards nitpicks to apply, but I don't see any of it myself, the patch seems fine as-is.
Comment #25
alexpottShouldn't we throw a runtime exception here if isValidBin not met?
Patch looks sane to me... but we need to test the isValidBin function...
Comment #26
markpavlitski CreditAttribution: markpavlitski commented@alexpott I'll add some tests to the next patch.
Other
DrupalDatabaseCache
functions, namelyget()
andset()
, fail silently rather than producing an error. Do we definitely want to throw an exception here?Edit: They fail silently when a database issue occurs, rather than user/caller error, so probably we do want to throw an error here.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedThe substr() call looks to me like it has the wrong parameters, and there's no need to "see issue X" in documentation (if there's any thing extra that needs to be said, just add it in the documentation)...
Otherwise, I agree that silently failing here doesn't seem right (@markpavlitski: the code comment says "other cache operations throw a PDO error in this situation", so is it wrong?)
Honestly, I wonder if the simplest way to deal with this issue isn't just to change db_truncate() to something like
db_delete($this->bin)->where('cid = cid')->execute()
? Then we get the PDO exception thrown automatically... A little bit of a hack, but in some ways it's cleaner than loading up the whole schema and checking it.Comment #28
catchA db_delete() is going to be a lot slower than a db_truncate() if you have a lot of rows, this is why db_truncate() uses native TRUNCATE if it's available.
Comment #29
markpavlitski CreditAttribution: markpavlitski commented@David_Rothstein You're right about the
substr()
call, I'll correct this.Using
db_delete()
could introduce a serious performance penalty over db_truncate, especially on larger tables such as{cache_form}
.The PDO exception comment refers to the other
cache_clear_all()
operations, such as:In the situation mentioned by the OP, where
{block}
was used as the bin name, the cid field doesn't exist so would throw a PDO exception.Comment #30
pounard@#27 TRUNCATE operation with MySQL also offers the courtesy of freeing the table disk space, which DELETE won't necesarily do. It is also known to be faster in scenarios where the table has a high volume of data. I think TRUNCATE remains good to use here, cache tables are higly mutant and can potentially (think of cache_page and cache_block which are emptied every 3 hours) grow *a lot*, and a table on which you don't OPTIMIZE manually on MySQL is a table which may never reclaim disk space.
Comment #31
markpavlitski CreditAttribution: markpavlitski commentedThis patch throws an exception rather than failing silently, clarifies the documentation, and adds test coverage.
Comment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedTRUNCATE is probably a bad idea here, because (1) it's a DDL statement and has such has to lock *the whole table* to execute, and (2) it's awfully slower then a DELETE on mostly empty tables; but discussing this is not in the scope of this issue.
Comment #33
alexpottthis should throw a http://php.net/manual/en/class.runtimeexception.php
Comment #34
markpavlitski CreditAttribution: markpavlitski commented@alexpott I can't see a RuntimeException being thrown anywhere else in the D7 codebase, only in D8. Can you confirm?
Comment #35
alexpottOh ok... had my Drupal 8 head on :) ... it's no biggie so leave as is
Comment #36
alexpottDidn't mean to make this minor... it's just not critical :)
Comment #37
markpavlitski CreditAttribution: markpavlitski commentedOops, didn't mean to change priority... Thanks for clarifying!
Comment #38
pounardSwitching back #31 to RTBC.
Comment #39
pounardOups didn't see the debate about Exception vs RuntimeException sorry.
Comment #40
markpavlitski CreditAttribution: markpavlitski commented@pounard I think that was agreed- Exception is fine for D7 (see #35).
Comment #41
pounardOh OK, so back RTBC for me.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedAlright, let's do this. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/f019275
I removed the @see pointing to this issue on commit (since there's still no need for that), and also removed t() from the test assertion messages.
Comment #43
pounardGreat! Thank you very much for this.