Hello!

We're using revisions on our stories here, and those can get quite big. We have just stumbled upon a case where the size of the revisions field would overflow the assigned maximum (2^16-1 == 65536 chars), creating an invalid revisions field that the unserialize() function cannot parse.

Took us a while to track the bug down. The symptom was that the webserver would crash spontaneously at apparently random places, but we've been able to track it to that single article which had too many revisions. Apparently, Drupal tries to access the node->revisions while it is invalid, which makes PHP panic.

Of course, PHP shouldn't croak like this, but this is another story.

The ugly workaround we have found right now was to:

1- clear the revision field so that the article gets displayed again
2- promote the revisions field to a MEDIUM- or LONGTEXT (not sure which yet)
3- apply the following, untested and lousy but necessary patch:

--- node.module 2003-08-06 16:29:26-04 1.1
+++ node.module 2003-08-06 16:29:57-04
@@ -243,6 +243,9 @@

if ($node->revisions) {
$node->revisions = unserialize($node->revisions);
+ if ($node->revisions === FALSE) {
+ die("invalid revisions field");
+ }
}

/*

Additionally, the revisions field shouldn't be allowed to be saved to if the serialized data is bigger than the field size, because it causes data corruption.

The above applies (I think) to 4.1 and up, testing on FreeBSD/Apache 1.3.27, PHP 4.3.1.

Comments

anarcat2’s picture

anarcat2 commented

Here is a more elegant fix:

--- node.module 6 Aug 2003 21:55:55 -0000 1.38
+++ node.module 5 Sep 2003 14:47:56 -0000
@@ -228,7 +228,8 @@
if ($node->revisions) {
$node->revisions = unserialize($node->revisions);
if ($node->revisions === FALSE) {
- die("unserialize failed");
+ watchdog("error", "$node->type: '$node->title' ($node->nid) cannot unserialize() revisions");
+ $node->revisions = "";
}
}

When the revisions field is invalid, this makes drupal recover cleanly, but will destroy the (corrupted) revisions on the next run.

I'll see if I can keep this from happening in the first place by checking the size of the revisions field (how the heck can I do that?) before serializing the revisions.

killes@www.drop.org’s picture

killes@www.drop.org commented
Assigned: Unassigned » killes@www.drop.org

This bug report was against 4.1. I'll upload the patch to cvs.

killes@www.drop.org’s picture

killes@www.drop.org commented

a patch has been uploaded to the for_review directory (#0112)-

moshe weitzman’s picture

moshe weitzman commented

killes - is yuour patch still valid. have you received feedback on it. please persist in landing the patch

killes@www.drop.org’s picture

killes@www.drop.org commented

The patch still applied last week, I did not get any response, but I am confident that it will get applied sooner or later.

dries’s picture

dries commented

Is the patch still necessary? We changed the database field to type 'text'.

killes@www.drop.org’s picture

killes@www.drop.org commented

CREATE TABLE node (
nid int(10) unsigned NOT NULL auto_increment,

teaser text NOT NULL,
body mediumtext NOT NULL,
changed int(11) NOT NULL default '0',
revisions text NOT NULL,

According to
http://www.mysql.com/doc/en/Storage_requirements.html
mediumtext is longer than text, thus the problem still can occur. Even if revisions were changed to be mediumtext the patch would still be neccessary to avoid overflow.

anarcat’s picture

anarcat commented

Whatever the size of the field, the patch is still necessary. At *some* point, given a sufficiently large amounts of changes to a revisioned node, the field will overflow and Drupal will crash mod_php.

(Hey! I got my real account back! :)

killes@www.drop.org’s picture

killes@www.drop.org commented
Assigned: killes@www.drop.org » Unassigned

0182.HEAD.anarcat.revisions-unserialize-failure-fallback.patch

should be applied to fix this issue.

Kjartan’s picture

Kjartan commented
Priority: Critical » Minor
anarcat’s picture

anarcat commented

Why? Why is this being simply dropped on the floor?

Should I see the "won't fix" status as a refusal to adopt good coding practices, in that case checking for a function return values?

I don't understand.

moshe weitzman’s picture

moshe weitzman commented

nothing is being dropped on the floor. the maintainers of this project evaluated this patch and have actively chosen not to incorporate it.

this patch adds error handling around a highly unlikely error. so the maintainers decided that the crufty error handling is worse than sufferring an extremely unlikely bug.

anarcat’s picture

anarcat commented

Thank you for explaining why this bug was closed. There was no precision given when the bug was closed, so that is why I "jumped" like this. :) Sorry for loosing my temper.