Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.This won't behave the same way in MySQL, since the spam and notspam fields are set to unsigned for MySQL, but since that's a proprietary SQL extension, it's not portable to PostgreSQL (and I'm also not sure exactly what the behaviour actually *is* under MySQL when you attempt to decrement an unsigned integer below zero -- does it simply floor at zero, or does it roll over to sizeof(integer)-1?).
In any case, the attached patch both sanity checks the spam and notspam values during spam_save and spam_unsave, and logs a watchdog warning when it happens to make the problem more visible. It also forces the token existence check to check the return of the query, and not the values of the spam or notspam elements.
I suspect that there is a logic error somewhere in whatever decides whether to call spam_unsave on a token that allows this to happen even if the token has never before been tagged as spam (or notspam), and thus has a base value of zero. If MySQL unsigned ints simply floor at zero, then this behaviour is simply, subtly, and silently corrupting the spam_tokens table by incrementing the opposing value by one, and never decrementing the one that would have dropped below zero.
On PostgreSQL, the problem is more serious, as values can in fact go negative... and that wreaks havoc on anything that checks spam or notspam as a bool to test for existence, as well as messing with the probability value checks, causing division by zero errors and incorrect tagging.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | spam_1.pgsql | 3.33 KB | Zed Pobre |
| #9 | spam.module_4.patch | 2.53 KB | jeremy |
| #6 | spam.module_3.patch | 1.96 KB | jeremy |
| #2 | fixnegative_0.patch | 3.15 KB | Zed Pobre |
| fixnegative.patch | 3.34 KB | Zed Pobre |
Comments
Comment #1
jeremy commentedI want to play around with this a little and understand why the values are going negative in the first place...
Comment #2
Zed Pobre commentedNew patch shortening the watchdog text, which truncates.
Comment #3
jeremy commentedAre you seeing negative values still? (That is, are you seeing your new watchdog entries?)
Comment #4
Zed Pobre commentedYes. It triggers a whole slew of them just about every time I manually mark a trackback as spam. It doesn't seem to be triggering on automatic trackback deletion (a little hard to tell now that so many trackbacks are being blocked by IP, so the incidents are getting much further apart), and never seems to trigger on comments being marked spam or not spam.
Comment #5
jeremy commentedI did not realize it was related to trackbacks. That explains why I've been unable to duplicate the problem. I'll set up a trackback testlab and see where the actual bug is. Thanks.
Comment #6
jeremy commentedHow about this patch instead. It simply prevents ever saving a negative spam or notspam value. (If you already have negative values, you'll need to manually fix them.)
Comment #7
Zed Pobre commentedUh, well, that looks like it will probably also work... but if you were concerned about patches that just hide the underlying problem, you're back to exactly the same effect as my original patch. Something is causing spam_unsave_token to be called on tokens that had never previously been spam (or notspam, depending), and that is going to artificially increase the opposing value even if one side is being stopped from going negative.
Comment #8
Zed Pobre commentedAlso, I've decided that I really like the notion of logging warnings whenever invalid values are found, I think, though I need to dig through the Drupal docs (such as they are) and find out how to better use watchdog and get around the truncation.
Comment #9
jeremy commentedYeah, whoops. The patch is updated to use the spam_log() facility to note when a negative value would have been created.
(FWIW: I created spam_log() because in 4.5 the watchdog() function is not very useful for non-core modules. I believe in 4.7 it has been improved allowing me to easily view just spam messages, so when I fork the module for 4.7 I'll look into getting rid of the special spam_log table, and having the spam_log() function write a watchdog entry instead. Not something to worry about in this patch.)
Comment #10
Zed Pobre commentedActually, the watchdog log should allow you to do that *now*... what problem are you having with it, exactly? My only problem is that it truncates sharply, and seems to have a very finicky call syntax.
Comment #11
jeremy commentedIf I remember correctly, I was unhappy as there was no simple built-in way to display only spam logs, nor to set various levels of logging. It's a non-issue for 4.5/4.6, as I'm not changing this release to use the watchdog, the spam_log facility works fine. I will revisit this again when I fork the 4.7 release. I plan to merge this an release a new version of the spam module tomorrow.
Comment #12
Zed Pobre commentedI just altered the spam.pgsql file to force constraints on the database that will cause it to throw a nasty error message if spam or notspam try to go < 0, or if probability goes <= 0 (this also sets probability default to 1). This, at least, should absolutely prevent any database corruption even if further bugs show up in the code.
You may want to set the probability to default 1 on the spam.mysql file as well.
Comment #13
jeremy commentedPatches merged, 2.0.12 released. Thanks.
Comment #14
(not verified) commented