718247 - 500 Internal Server Error trying to load the mini-graph for certain signatures/top frames

Status: VERIFIED FIXED

(Socorro :: Webapp, task)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot

STR:

1. In prod, load https://crash-stats.mozilla.com/topcrasher/byversion/Firefox/10.0b3/28/all
2. Click on the graph-icon for the 12th result
3. Wait

Expected Results:

Graph loads

Actual Results:

[17:13:39.663] GET https://crash-stats.mozilla.com/topcrasher/plot_signature/Firefox/10.0b3/mozalloc_abort(char%20const*%20const)%20%7C%20mozalloc_handle_oom()%20%7C%20nsTArray_base%3CnsTArrayDefaultAllocator%3E::EnsureCapacity(unsigned%20int,%20unsigned%20int)%20%7C%20nsTArray%3CgfxGlyphExtents*,%20nsTArrayDefaultAllocator%3E::AppendElements%3CgfxGlyphExtents*%3E(gfxGlyphExtents*%20co.../2011-12-16/2012-01-13/ [HTTP/1.1 500 Internal Server Error 138ms]

Comment 1

[Matt Brandt [:mbrandt]]

Stephend, interesting find. I wonder what's different about the 12th result. Result 11, 13, 14 work; but not the 12th.

Comment 2

[Robert Helmer [:rhelmer]]

Exception from the logs:

2012-01-14 19:58:23 -08:00 --- error: [5xx Error] File: application/controllers/topcrasher.php; Line: 289; Message: Missing argument 5 for Topcrasher_Controller::plot_signature()


(In reply to Matt Brandt [:mbrandt] from comment #1)
> Stephend, interesting find. I wonder what's different about the 12th result.
> Result 11, 13, 14 work; but not the 12th.

That signature has a lot of particularly gnarly characters in it, I wonder if it's not being escaped properly </peanut_gallery>

Comment 3

[Brandon Savage [:brandon]]

This one's mine, we pushed it out to 2.4.1 because it wasn't a regression, but it's still bad.

Comment 4

[Schalk Neethling [:espressive]]

Bug is as follows:

Any signature that ends with a ... causes an error

Comment 5

[Schalk Neethling [:espressive]]

So, here is the details of what is happening:

As stated above, if a signature ends with dots the error occurs. The reason is that Kohana, for security reasons, removes any dot paths such as ./ or ../

The following signature and URL used for the Ajax request demonstrates the problem:

sig: mozalloc_abort(char%20const*%20const)%20%7C%20mozalloc_handle_oom()%20%7C%20nsTArray_base%3CnsTArrayDefaultAllocator%3E::EnsureCapacity(unsigned%20int,%20unsigned%20int)%20%7C%20nsTArray%3CgfxGlyphExtents*,%20nsTArrayDefaultAllocator%3E::AppendElements%3CgfxGlyphExtents*%3E(gfxGlyphExtents*%20co...

URL: https://espressive-dev.allizom.org/topcrasher/plot_signature/Firefox/10.0b3/mozalloc_abort(char%20const*%20const)%20%7C%20mozalloc_handle_oom()%20%7C%20nsTArray_base%3CnsTArrayDefaultAllocator%3E::EnsureCapacity(unsigned%20int,%20unsigned%20int)%20%7C%20nsTArray%3CgfxGlyphExtents*,%20nsTArrayDefaultAllocator%3E::AppendElements%3CgfxGlyphExtents*%3E(gfxGlyphExtents*%20co.../2012-01-08/2012-01-15/

As you can see from the URL there is clearly 5 parameters but, because of the dots in the signature, Kohana will do it's chopping of the URL and the one that actually get's sent to the server look s follows:

https://espressive-dev.allizom.org/topcrasher/plot_signature/Firefox/10.0b3/mozalloc_abort(char%20const*%20const)%20%7C%20mozalloc_handle_oom()%20%7C%20nsTArray_base%3CnsTArrayDefaultAllocator%3E::EnsureCapacity(unsigned%20int,%20unsigned%20int)%20%7C%20nsTArray%3CgfxGlyphExtents*,%20nsTArrayDefaultAllocator%3E::AppendElements%3CgfxGlyphExtents*%3E(gfxGlyphExtents*%20co2012-01-08/2012-01-15/

Hence there only 'seems' to be four parameters. Now, simply stripping the dots from the signature will not help as we will just receive a result with no... results.

Now, apparently creating a file or adding the following to an existing file in application/classes/routes.php can prevent this:

<?php defined('SYSPATH') or die('No direct script access.');

class Route extends Kohana_Route {

    const REGEX_SEGMENT = '[^/,;?]++';

}

but then it is also suggested that the above is not the way to go an one should do the following:

Route::set('example', '<controller>(/<method>(/<id>))', array('id' => '[^/,;?]++'));

The latter unfortunately seems to be a new way of defining routes in Kohana 3 which we do not run. With that said though, neither of the above is really recommended as it might have some other unexpected side effects.

The main suggestion therefore is simply to avoid dots in URLs. As far as I can tell though, those signatures come back from the DB with the dots already added and there is nothing in the code that actually adds them based on some condition. This leads to the question of, how much effort would it be to have a script run through the relevant table and remove all trialing dots from signatures and, then prevent them from being added in future to any new crash signature?

There is another option but, as Kohana is doing this for security reasons it is probably not a good idea. Anyway, the other option is, in Kohana_Request there is the following code:

// Remove all dot-paths from the URI, they are not valid
$uri = preg_replace('#\\.[\\s./]*/#', '', $uri);

We could amend this to not strip those patterns from URL's and handle it some other way. This is probably a last resort and not something that would be taken lightly but, that is where we are at with this bug.

Suggestions on how to proceed?

Comment 6

[Robert Kaiser]

We can't just truncate the "..." as it actually tells us that we left out stuff in those signatures. Could we always append some unambiguous character to the signature in those calls so that the "..." doesn't touch the "/" and therefore avoid this unfortunate Kohana problem?

(I'd understand if they'd cut '#/\\.[\\s./]*/#' for security but without another leading slash in the regex they are over-zealous there.

Comment 7

[Schalk Neethling [:espressive]]

Sent pull request:
https://github.com/mozilla/socorro/pull/285

TEST CASE
----------

Go to a page such as:
https://espressive-dev.allizom.org/topcrasher/byversion/Firefox/10.0b3

Find a signature that ends with ... Easiest way is to hover over the longer signatures. Clicking on these previously would cause the 500 error as mentioned by this bug. 

The result now will be that the minigraph successfully loads with data.

Comment 8

[[github robot]]

Commits pushed to https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/6550d63db8a0e6ccd2fac29194fa59a8ccd7f813
moved sig to end of URL fixes bug 718247

https://github.com/mozilla/socorro/commit/52571a0871c56473c752b87e518753c42c1d532a
Merge pull request #285 from ossreleasefeed/bug718247-minigraph-server-error

moved sig to end of URL fixes bug 718247

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Post-fix screenshot

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED:

--
[11:19:38.145] GET https://crash-stats-dev.allizom.org/topcrasher/plot_signature/Firefox/9.0.1//2011-12-20/2012-01-17/hang%20%7C%20mozilla::plugins::PPluginInstanceParent::CallPBrowserStreamConstructor(mozilla::plugins::PBrowserStreamParent*,%20nsCString%20const&,%20unsigned%20int%20const&,%20unsigned%20int%20const&,%20mozilla::plugins::PStreamNotifyParent*,%20nsCString%20const&,%20nsCString%20const... [HTTP/1.1 200 OK 473ms]