Big bug in management comments

moved to drupal project, as this is a core issue, not my poor lil module.

the bug should be fixed in cvs first and backported soon after.

The actual reason all comments are deleted is that a empty value gets passed to _comment_delete_thread() for the ->cid value. The API states that "NOTE: using this syntax will cast NULL and FALSE values to decimal 0, and TRUE values to decimal 1."

So, the WHERE clause gets translated to "WHERE pid = 0" and this, recursively, ends up wiping all the comments.

The attached patch stops this happening in _comment_delete_thread() so will at least protect your site against this type of "race condition".

However, the UI should detect it eariler in the process before calling _comment_delete_thread()

I haven't looked at doing that just yet. I thought I would get this minor patch out for those that "want it now" to at least provide protection.

regards
--AjK

I'd rather see a fix in the calling code

  if ($comment->cid && $_POST['edit']['confirm']) {

There should probably be something right next to where $comment is set, since it may be NULL and is used as an object on the next line (we should never use undefined variables as objects).

The watchdog message needs some help as well. People are going to be reading it, not programmers.

>> "code needs work" for sure. I didn't intend that patch for core but was meant for worried admins who wanted to effect at least protection from what is effectively "delete from comments;" when a null value is passed. Sorry I didn't set the status of the issue correctly.

I have actually started looking at the fix in the calling section so that it shouldn't even occur however, because db_query() casts a NULL or FALSE to 0 for %d type, I belive checking the value before calling db_query() is important. I actually thought there was a bug in db_query() until I re-read the API doc where it's plain to see it's a feature.

On a side note, I have seen a few issues regarding the loss of the UID = 0 account (anon). This as actually happened to me once and I put it down to "finger problems" in our London office. However, now understanding db_query()'s cast to zero feature, I'm not so sure. So I may go looking for "unprotected" sql where a zero might not have been expected and db_query()'s subsequent effects not noticed.

I must say, although db_query()'s effetcs are documented in the API, I can't help thinking that this often goes unnoticed. It may be that for %d type vars passed, db_query() should do is_int($...) || ctype_digit($...) and enforce a number for %d (ie change the feature from "casting to zero" to "error"). But that may cause more problems that it solves. Depends on how the many people have been using db_query() and if people have actually relied on this feature (it would break their code to change it). But I'd be surprised to learn that people have actually "relied" on this for operational code.

best regards
--AjK

OK, here's my patch that catches comments that have been deleted during this "race condition" bewteen two or more administrators that are deleting comments.

I've tested it under a number of conditions and it appears to do what's required. Over for review.

best regards
--AjK

I should have taken drumm's advice a bit more. Here's a patch as above, however, as drumm suggests, this checks the $comment for object type and cid validity.

best regards
--AjK

having chatted by drumm on IRC he made some recommendations. I have applied these and retested and it all appears ok to me now (and hopefully to drimm ;)

Patch attached.

best regards
--AjK

Is it me or does this patch more than what is strictly necessary. Please provide a minimal patch that we can backport to DRUPAL-4-7.

Aren't many of these ctype_digit($comment->cid) checks redundant/excessive? It's not something we do in other modules?

Also, why do we use ctype_digit()? Can't we use is_numeric() like we use elsewhere?

The patch does two things.

1. It "protects" the SQL in the recusive _comment_delete_thread() function from a malformed $comment. Without this protection the ->cid can be null, db_query() casts this to zero 0. In doing so, this cast effectively means "delete from comments;" albeit a slower version (deletes all by recursion)
2. It fixes the root cause however, by ensuring that when the _confirm page is called, any deleted comments in the meantime don't get put into the confirm form, prevent the call with null. Note, this really on reduces the scope of the race condition to a smaller window of opertunity. I believe there may still exist a race condition but it's chances of happening are much reduced. However, should it occur, 1) above will protect the sql.

So, the patch is, imho, doing realistically the minimal amount to fix the problem.

As for your other comments, I have sent an email to yourself, "killes" and "heine" as it's all sometwhat long winded explanation. However, if there exists "a way thing are to be done" I'm happy to do things "that way".

I will look at DRUPAL-4-7 shortly. Sorry, being new to core patching, I forgot that a bug fix needs backporting.

best regards,
--AjK

AjK: I'm currently traveling and somewhat behind e-mail.

Dries, yes, saw your blog so appreciate the delay.

Have a good trip (and sorry for the off topic issue post)

best regards
--AjK

Dries, actually we should use ctype_digit everywhere because it is faster than is_numeric

chx,

thanks for the follow up. I sent Dries an rather long winded email regarding ctype_digit() amoungst other things. It's true the ctype_digit() is faster than is_numeric() but this is only true if the var being passed is a string representation of a decimal digit. If the var being passed is a true int (zval->type == IS_LONG) then is_numeric() turns out to be faster than ctype_digit().

It's all about knowing what type of $var you are working with.

I used ctype_digit() as I know it was a string representation being tested and not a true int.

best regards
--AjK

I'm recompiling my PHP (on Gentoo) to support ctype_*().

Maybe in the future we will need a ctype gap-filler if it isn't installed, like MediaWiki apparently has and like we have done for unicode support.

Committed to HEAD.

Ctype functions are available on drupal.org. According to php.net they are enabled by default from 4.2.0 on.

backported to 4.7.