1354433 - treeherder's bug filer created bugs with massive and incorrect crash-signatures

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect)

Description

[:glob ✱]

treeherder's bug filer created bugs with massive and incorrect crash-signatures.
this broke a few things in bmo land, which are in the process of being cleaned up.

eg. https://bugzilla.mozilla.org/show_activity.cgi?id=1353512

in case the history is removed from that bug, the crash signature consisted of "[@ mozalloc_abort]" repeated 227,106 times.  i suspect this is wrong.

Comment 1

[GitHub Autolander Bot]

Attachment: [treeherder] KWierso:maxcrash > mozilla:master

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8855644 [details] [review]
[treeherder] KWierso:maxcrash > mozilla:master

So I forgot that staging Treeherder is still set up to submit to production bugzilla, and accidentally filed more than one bug while trying to figure out how long the cf_crash_signature field can be. Apparently it can be ridiculously long, and if it *is* ridiculously long, you'll inadvertently DOS production bugzilla when trying to load those bugs...

So here's a totally arbitrary cap at 2048 characters on both the bug filer's form and on the bug filing backend service, to hopefully stop this from happening, at least from Treeherder's bug filer.

Comment 3

[Ed Morley [:emorley]]

Comment on attachment 8855644 [details] [review]
[treeherder] KWierso:maxcrash > mozilla:master

Left some comments :-)

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8855644 [details] [review]
[treeherder] KWierso:maxcrash > mozilla:master

Test added, and fixed the NoneType error that was in there.

Comment 5

[Ed Morley [:emorley]]

Comment on attachment 8855644 [details] [review]
[treeherder] KWierso:maxcrash > mozilla:master

Wes, just to check I understand correctly:
* you were testing something using the bug filer and in so doing some bugs with really long crash signatures were filed
* in normal usage such values would not normally be generated
* even though the BMO schema and/or API validation allows these long values, they cause performance issues for BMO

It seems like:
1) BMO's API should prevent such long values being used if they are problematic.
2) If the bug filer were routinely creating such values, we should also be a good citizen and prevent them via the attached PR.

However since this was a one-off, do we need to do #2? We don't have Treeherder API validation of eg max summary length?

I'm fine either way, just wanted to check.

Comment 6

[:glob ✱]

(In reply to Ed Morley [:emorley] from comment #5)
> 1) BMO's API should prevent such long values being used if they are
> problematic.

yes - a few bugs have been filed to fix up BMO's side of this outage.
reducing the max size for the crash-signature is one.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Ed Morley [:emorley] from comment #5)
> Comment on attachment 8855644 [details] [review]
> [treeherder] KWierso:maxcrash > mozilla:master
> 
> Wes, just to check I understand correctly:
> * you were testing something using the bug filer and in so doing some bugs
> with really long crash signatures were filed
> * in normal usage such values would not normally be generated
> * even though the BMO schema and/or API validation allows these long values,
> they cause performance issues for BMO
If you submit a summary that's too long, you get an error message along the lines of "summary exceeded max length of 255 characters". I was trying to see if the API similarly described the max accepted length of the field, so I sent in what I had hoped was a too-long string. But the API accepted it, causing these issues. 
Normally, the value would either be an empty string, undefined, or whatever length one or more crash signature fields concatenated ended up being. Probably no more than a few hundred characters.

> However since this was a one-off, do we need to do #2? We don't have
> Treeherder API validation of eg max summary length?
We could probably get away with just having it be capped on the UI side and not validated it on the API end, too. Was a bit concerned that someone could maliciously edit the request sent to the API, causing more DoS issues, but hopefully BMO fixes their side of things before that'd be a real issue.

Comment 8

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/902a22a2533de14220797b9afa742730712c5abc
Bug 1354433 - Restrict crash signature field to 2048 characters (#2325) r=emorley

* Bug 1354433 - Restrict crash signature field to 2048 characters

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Went ahead and landed this with the API-side enforcement.