256592 - DBD errors (especially "packet too large") kill browser

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[:glob ✱]

if you try to add an attachment that is greater than mysql's
'max_allowed_packet' variable, the resulting error message contains the
attachment as text, as it contains the sql that failed.  this kills firefox on
my pc.

we should truncate DBI error messages to avoid this.

Comment 1

[:glob ✱]

Attachment: dbd errors v1

sets maximum length of dbd errors to 4096 chars.  no real reason for that
particular value :)

Comment 2

[Joel Peshkin]

Will this still put the full error message somewhere?  It is important that it
be accessible.  If we truncate the message to the browser, let's make sure it
goes in the webserver error log.

Comment 3

[Jouni Heikniemi]

If we set the limit to 10000 bytes, is it really important to have it in
webserver log? I mean, are there any practical conditions where SQL after 10000
chars would be relevant even for the admin (the only case for that is probably
the attachment thing anyway - flooding the attachments into the webserver log
doesn't sound necessary to me anyway).

Comment 4

[:glob ✱]

jouni, that was exactly my thinking, i just picked a lower value :)

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 156806 [details] [diff] [review]
dbd errors v1

Works. Good to have -- I agree with Byron and Jouni.

Comment 6

[Marc Schumann [:Wurblzap]]

Applies cleanly on branch, too.

Comment 7

[Dave Miller [:justdave]]

For the record, this is a regression.  This bug was previously reported as bug
57819, and was fixed by Gerv with a patch checked in on 2001-10-13.  The code
was subsequently lost when the DB stuff got ported to its own module by bbaetz
on 2003-01-14.  Clearing the approval flags because I plan to deny review on
this patch for the same reason I made Gerv back out his initial fix to bug 57819.

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 156806 [details] [diff] [review]
dbd errors v1

We need to keep the end of the error message, because that's where the actual
error is given.  Ideally we need to truncate from the middle and keep both ends
of the error.  The original fix for this that got lost kept 2K off each end and
added an elipsis (...) in the middle.

Comment 9

[:glob ✱]

Attachment: dbd errors v2

truncate middle of dbd errors.

Comment 10

[Vlad Dascalu]

Comment on attachment 167916 [details] [diff] [review]
dbd errors v2

+    $errstr = substr($errstr, 0, 2000) . ' ... ' . substr($errstr, -2000)
+	 if length($errstr) > 4000;

Well I guess we could use substr($_[0], 0, 2000) and such for performance
reasons, but this works as well I guess.

Comment 11

[:glob ✱]

it's a fatal database error, i don't think performance is critical :)

Comment 12

[Vlad Dascalu]

(In reply to comment #7)
> for the same reason I made Gerv back out his initial fix to bug 57819.

Dave, I don't know about you, but I don't see anywhere in bug 57819 anything to
indicate that Gerv backed out its original patch. As far as I can see, an
improvement uploaded as the second patch was applied over it. The original patch
was never backed out.

Comment 13

[Vlad Dascalu]

(In reply to comment #11)
> it's a fatal database error, i don't think performance is critical :)

$_[0] could contain the whole attachment, which could be a .jpg or something
else, 1 MB wide or something like that. That could take a while to copy from one
place to another... :)

Comment 14

[:glob ✱]

Attachment: dbd errors v3

Comment 15

[Vlad Dascalu]

Comment on attachment 167920 [details] [diff] [review]
dbd errors v3

This one rocks, no $errstr at all! :)

Comment 16

[Dave Miller [:justdave]]

(In reply to comment #12)
> Dave, I don't know about you, but I don't see anywhere in bug 57819 anything to
> indicate that Gerv backed out its original patch. As far as I can see, an
> improvement uploaded as the second patch was applied over it. The original patch
> was never backed out.

Technicalities!  It was the thought that counts, or something like that ;)

Comment 17

[Vlad Dascalu]

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.12.2.2; previous revision: 1.12.2.1
done

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.14; previous revision: 1.13
done