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

VERIFIED FIXED in 2.4.1

Status

VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: stephend, Assigned: espressive)

Tracking

unspecified
2.4.1

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

Stephend, interesting find. I wonder what's different about the 12th result. Result 11, 13, 14 work; but not the 12th.
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>
This one's mine, we pushed it out to 2.4.1 because it wasn't a regression, but it's still bad.
Target Milestone: --- → 2.4.1
Assignee: nobody → bsavage
(Assignee)

Updated

7 years ago
Assignee: bsavage → sneethling
(Assignee)

Comment 4

7 years ago
Bug is as follows:

Any signature that ends with a ... causes an error
(Assignee)

Comment 5

7 years ago
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

7 years ago
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.
(Assignee)

Comment 7

7 years ago
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

7 years ago
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

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 9

7 years ago
Created attachment 589581 [details]
Post-fix screenshot
Duplicate of this bug: 717771
You need to log in before you can comment on or make changes to this bug.