679439 - usebuildbot has broken links for "Analyze the leak"

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Phil Ringnalda (:philor)]

In http://tbpl.allizom.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=47ae156df73f the OS X debug mochitest-other has a leak, with "Analyze the leak" links going to http://tbpl.allizom.org/leak-analysis/?id=5992358&tree= which produces "invalid tree" and if you alter that to http://tbpl.allizom.org/leak-analysis/?id=5992358&tree=Mozilla-Inbound produces "invalid id."

Comment 2

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

A new version of tbpl was rolled into production on 31aug. I just looked at https://tbpl.mozilla.org/?usebuildbot=1&rev=2cbb9f7ffe32, which has a leak, and afaict, the links to leak info is just fine.


Is this still an issue, or can we now close this?

Comment 3

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/tip/leak-analysis/index.php

Comment 4

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&onlyunstarred=1&rev=b1fbce31b7f2 would be a better example, since it's not starred and will not be starred, so the links, which were no longer visible on that m-c leak by the time you were looking at it, will still be visible.

STR:
1. Click the orange "oth" on the Win debug line.
2. In the blue popup that appears in the lower right when you do that, there are six identical links with the text "Analyze the leak" all of which go to https://tbpl.mozilla.org/leak-analysis/?id=6320266&tree=
3. Either click one, or check the link in comment 3 to see that it still requires a tree param which contains one or more letters, numbers, dots and/or dashes, and an id param which consists of three groups of one or more digits separated by dots, ending in .gz.

Comment 5

[Robert Helmer [:rhelmer]]

Attachment: add "analyze this leak" support buildbot

* pass tree name param to leak-analysis/index.php (although this isn't really used, I suppose leak-analysis/index.php could be changed to make this not required)

* remove regex check on ID in leak-analysis/index.php (buildbot IDs are totally unlike tinderbox ones)

* add support for cached logs (buildbot-style) to leak-analysis/index.php, with a fallback to tinderbox if "tinderbox=1"

* add "&tinderbox=1" param in link to leak-analysis/index.php from getTinderboxSummary.pyp

Comment 6

[Arpad Borsos [:Swatinem]]

Comment on attachment 559723 [details] [diff] [review]
add "analyze this leak" support buildbot

Review of attachment 559723 [details] [diff] [review]:
-----------------------------------------------------------------

::: leak-analysis/index.php
@@ -6,5 @@
>  if (!preg_match('/^[a-zA-Z0-9\.-]+$/', $_GET["tree"]))
>    die("invalid tree");
>  
> -if (!preg_match('/^\d+\.\d+\.\d+\.gz$/', $_GET["id"]))
> -  die("invalid id");

Make sure to include this check in the tinderbox path, to avoid exploits.

@@ +19,5 @@
> +  if (isset($_GET["usetinderbox"])) {
> +    $usetinderbox = $_GET["usetinderbox"];
> +  }
> +
> +  $fp = NULL;

Move other declarations like $lines, $fileExistsAfterAll and others here, they are valid in both paths.

@@ +20,5 @@
> +    $usetinderbox = $_GET["usetinderbox"];
> +  }
> +
> +  $fp = NULL;
> +  if ($usetinderbox==1) { 

Whitespace please.

@@ +38,5 @@
> +    $fileExistedAfterAll = false;
> +    $windows = array();
> +    $lastTestName = '';
> +  } else {
> +    $log_filename = "../cache/rawlog/" . $id . ".txt.gz";

Also, in this path, check $id to make sure its an int. Otherwise it can open arbitrary files on the FS.

::: php/getTinderboxSummary.php
@@ +148,4 @@
>    if (count($parts) < 2 &&
>        preg_match('/^leaked/i', $tokens[2])) {
>      $lines[] = "<a href=\"leak-analysis/?id=" . $_GET["id"] .
> +      "&usetinderbox=1" . "&tree=" . $_GET["tree"] . "\" target=\"_blank\">Analyze the leak.</a>";

I thought we are going the usebuildbot route here.

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to Arpad Borsos (Swatinem) from comment #6)
> Comment on attachment 559723 [details] [diff] [review]
> add "analyze this leak" support buildbot
> 
> Review of attachment 559723 [details] [diff] [review]:
...
> Make sure to include this check in the tinderbox path, to avoid exploits.
> 
...
> 
> Also, in this path, check $id to make sure its an int. Otherwise it can open
> arbitrary files on the FS.


Yikes, thanks for catching these; will fix these and the other issues as well.


> ::: php/getTinderboxSummary.php
> @@ +148,4 @@
> >    if (count($parts) < 2 &&
> >        preg_match('/^leaked/i', $tokens[2])) {
> >      $lines[] = "<a href=\"leak-analysis/?id=" . $_GET["id"] .
> > +      "&usetinderbox=1" . "&tree=" . $_GET["tree"] . "\" target=\"_blank\">Analyze the leak.</a>";
> 
> I thought we are going the usebuildbot route here.


It looks like TinderboxJSONUser.js uses php/getTinderboxSummary.php for annotatedSummaryURL, and BuildbotDBUser uses php/getLogExcerpt.php (I tested this patch on a local VM and it all seems to work ok). Let me know if this is wrong and/or I am misunderstanding your point.

Comment 8

[Arpad Borsos [:Swatinem]]

(In reply to Robert Helmer [:rhelmer] from comment #7)
> It looks like TinderboxJSONUser.js uses php/getTinderboxSummary.php for
> annotatedSummaryURL, and BuildbotDBUser uses php/getLogExcerpt.php (I tested
> this patch on a local VM and it all seems to work ok). Let me know if this
> is wrong and/or I am misunderstanding your point.

You are right, my bad.

As you mentioned, the buildbot part can live without the tree parameter, so we can remove it later in a followup.

Comment 9

[Robert Helmer [:rhelmer]]

Attachment: add "analyze this leak" support for buildbot

Address review comment 6:

* check both buildbot and tinderbox ID (tested this locally with some real/fake IDs, "../" etc.)
* move all common vars up above tinderbox/buildbot branch
* whitespace

Tested both the buildbot and "usetinderbox=1" route in a local VM, seems ok (buildbot route is much faster too since it uses the cached local file).

Comment 10

[Arpad Borsos [:Swatinem]]

Comment on attachment 559751 [details] [diff] [review]
add "analyze this leak" support for buildbot

Review of attachment 559751 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 11

[Robert Helmer [:rhelmer]]

Comment on attachment 559751 [details] [diff] [review]
add "analyze this leak" support for buildbot

Pushed:
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/726efca9b54b

I'll post a followup to remove the "tree" param in the buildbot case shortly.

Comment 12

[Robert Helmer [:rhelmer]]

Attachment: cleanup buildbot case

Leftover cleanup from attachment 559751 [details] [diff] [review]:

* do not pass "tree" param in buildbot case, we have the unique ID for the log so it's not used
* use $id consistently in analyze function once it's been passed in, instead of using the $_GET array again

Tested locally and the usual case as well as tweaking the "id" param seem to still work as expected.

Comment 13

[Arpad Borsos [:Swatinem]]

Comment on attachment 559777 [details] [diff] [review]
cleanup buildbot case

Review of attachment 559777 [details] [diff] [review]:
-----------------------------------------------------------------

::: leak-analysis/index.php
@@ +11,1 @@
>    $file = "../summaries/LeakAnalysis_" . $tree . "_" . $id;

$tree is used here. Maybe the cache file name should depend on usetinderbox.

Comment 14

[Robert Helmer [:rhelmer]]

(In reply to Arpad Borsos (Swatinem) from comment #13)
> Comment on attachment 559777 [details] [diff] [review]
> cleanup buildbot case
> 
> Review of attachment 559777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: leak-analysis/index.php
> @@ +11,1 @@
> >    $file = "../summaries/LeakAnalysis_" . $tree . "_" . $id;
> 
> $tree is used here. Maybe the cache file name should depend on usetinderbox.

Ugh right totally missed that... well we could leave this patch out in that case... having a cache file doesn't really make any difference in the buildbot case but it does save some work.

Comment 15

[Robert Helmer [:rhelmer]]

Closing out per comment 14, I think the current state is ok (would not object to making this code clearer, but that can probably wait until it's time to remove tbox support IMHO).

Reopen if you disagree.