This is due to a not-100%-robust conditional in path_redirect_cron():

  if ($purge = variable_get('path_redirect_purge_inactive', 0)) {
     $query = db_query("SELECT rid FROM {path_redirect} WHERE last_used < %d", time() - $purge);

(The snippet's from 6.x-1.0-rc2; the D7 HEAD code is equivalent.)

If the path_redirect_purge_inactive variable is a string that doesn't convert to an int, then the boolean test passes (most typical strings evaluate to boolean true), but when converted to an int in the time() - $purge expression, $purge is 0.

The variable actually got set incorrectly (to '3156300'; // 1 year --long story) on at least one project of mine, causing the site's first cron run to wipe out every entry in the table (over 200; luckily I had most backed up).

So, why is this a path_redirect bug? Because the conditional doesn't wholly represent the intention. I believe the following does:

  if (($purge = variable_get('path_redirect_purge_inactive', 0)) > 0) {

I'll post patches in a comment.

P.S. To elaborate a bit, here's output from a `php -a` session:

> php > echo (bool) "'31536000'; // 1 year.";
> 1
> php > echo (int) "'31536000'; // 1 year.";
> 0
CommentFileSizeAuthor
#1 path_redirect-D6-1061768-1.patch588 bytesadamdicarlo
#1 path_redirect-D7-1061768-1.patch629 bytesadamdicarlo

Comments

adamdicarlo’s picture

adamdicarlo commented
Title: Edge case causes path_redirect_cron() to incorrectly wipe all entries » [patches] Edge case causes path_redirect_cron() to incorrectly wipe all entries
Status: Active » Needs review
StatusFileSize
new path_redirect-D7-1061768-1.patch629 bytes
new path_redirect-D6-1061768-1.patch588 bytes

Here are patches for D6 (rolled off 1.0-rc2) and D7 (rolled off HEAD).

It's quite an edge case that might not happen to anyone else, but either way, I think a more deliberate (and safer) if condition is not a bad thing. :)

Status: Needs review » Needs work

The last submitted patch, path_redirect-D7-1061768-1.patch, failed testing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Postponed (maintainer needs more info)

I'm not sure how this is a problem though?

I ran this on both MySQL and PosgreSQL and latest Drupal 6 core:

if ($x = variable_get('non_existant_variable', '31536000')) {
  echo (time() - 31536000) . ' = ' . db_result(db_query("SELECT %d", time() - $x));
}

and both times I got 1266347452 = 1266347452 as the result, which was expected and shows this is working correctly.

Sounds like your problem was configuration error since you say the variable get set to '3156300' - which is actually only 5 weeks and 1 day.

adamdicarlo’s picture

adamdicarlo commented

Thanks for the fast response!

Yes, the problem was partly a configuration error -- the variable was set to exactly this:

'31563000'; // 1 year.

Exactly that. The entire thing. Not just '31563000'.

The problem is that in a conditional (without a relational operator, like >), that value is coerced to boolean by PHP. Again, this example:

php > echo (bool) "'31536000'; // 1 year.";
> 1

(TRUE).

However, when you use the value in an arithmetic expression, it's cast to int. So what is its value when cast to int?

php > echo (int) "'31536000'; // 1 year.";
> 0

So "time() - $purge" == time() - 0. If (int)$purge is 0, the if statement shouldn't be evaluated as true -- we shouldn't get to the "then" code. But we do, because of the form of the if statement -- "if ($foo)" when "if ($foo != 0)" or "if ($foo > 0)" was intended.

  if ($value)

and

  if ($value != 0)

are not always the same in PHP -- the if statement is written as if they are.

Here's a simplified version of your test showing the problem (Drupal and the DB have nothing to do with it):

if ($x = "'31536000'; // 1 year.") {
    echo (time() - 31536000) . ' = ' . (time() - $x);
}

The output I just got for it:
1266420287 = 1297956287

I hope I've clarified it. :)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)

Ah, so really there is nothing to fix here. The simple matter is we cannot support broken variables that have been set manually. If users were able to set it via the UI then we'd have a bug, but the reality is you can cause this kind of mayhem all over Drupal with broken or incorrect variables. As such, I'm marking this as works as designed.