1321349 - histogram.add doesn't check it's `this` object for the proper JSClass (histogram.add(1).add() crashes)

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Benjamin Smedberg]

STR-1:
Open the browser console and type in "Services.telemetry.getHistogramById("TELEMETRY_TEST_COUNT").add(1).add()"

STR-2:
Services.telemetry.getHistogramById("TELEMETRY_TEST_COUNT").add.apply(null, 1)

ACTUAL:
crash

The reason for the crash is that the telemetry code assumes the `this` object is the kind it expects, and treats its private as a Histogram*.

In order to do this safely, the function should check whether the `this` is the correct JSClass. Unfortunately the JSClass is currently declared static within a method here: http://searchfox.org/mozilla-central/rev/ef3cede9a5b050fffade9055ca274654f2bc719e/toolkit/components/telemetry/TelemetryHistogram.cpp#1694

So to fix this properly you need to move the JSClass definition into file scope so each method impl can compare it properly.

The reason STR-1 works is that many of the functions in this file don't set the return value, which is a (silent) API violation. Every JS method that returns `true` should set a return value. It should be `undefined` most of the time. So we should while we're touching this fix it so that this method and the others here call CallArgs.rval().setUndefined()

Because these methods are only available to chrome-privileged code, this is not a security issue, but it is something that should not be possible.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Thanks for the detailed report!

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #0)
> STR-1:
> Open the browser console and type in
> "Services.telemetry.getHistogramById("TELEMETRY_TEST_COUNT").add(1).add()"
> ...
> The reason STR-1 works is that many of the functions in this file don't set
> the return value, which is a (silent) API violation. Every JS method that
> returns `true` should set a return value. It should be `undefined` most of
> the time. So we should while we're touching this fix it so that this method
> and the others here call CallArgs.rval().setUndefined()

Good news, STR-1 was just fixed by bug 1315906.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1321349.patch

This fixes the STR-2, which can be reproduced with:

> Services.telemetry.getHistogramById("TELEMETRY_TEST_COUNT").add.apply(null, [1]);

Instead of the line in comment 0. I manually tested both this and the keyed version using the browser console in about:telemetry, everything seems to work (easy to check: it was crashing before, it isn't crashing now).

However, when trying to add test coverage, something funny came up. I added the following lines after [1], and the test would just "lock up". It doesn't crash (as expected) but, for some reason, it just never completes and times out.

> // Also make sure that calling add() with an invalid 'this' doesn't
> // crash the browser.
> Telemetry.getHistogramById("TELEMETRY_TEST_EXPONENTIAL").add.apply(null, [1]);
> Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT").add.apply(null, ["some-key", 1]);

Not sure why that's happening.

[1] - http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js#283

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #3)
> This fixes the STR-2, which can be reproduced with:
...
> However, when trying to add test coverage, something funny came up. I added
> the following lines after [1], and the test would just "lock up". It doesn't
> crash (as expected) but, for some reason, it just never completes and times
> out.

Is it only timing out without the patch?
If so, we can still add this as test coverage.

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #2)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #0)
> > STR-1:
> > Open the browser console and type in
> > "Services.telemetry.getHistogramById("TELEMETRY_TEST_COUNT").add(1).add()"
> > ...
> > The reason STR-1 works is that many of the functions in this file don't set
> > the return value, which is a (silent) API violation. Every JS method that
> > returns `true` should set a return value. It should be `undefined` most of
> > the time. So we should while we're touching this fix it so that this method
> > and the others here call CallArgs.rval().setUndefined()
> 
> Good news, STR-1 was just fixed by bug 1315906.

Scanning the code, this is missing for:
- internal_JSHistogram_Clear
- internal_JSKeyedHistogram_Clear

Can we fix those too & add systematic test coverage for all JSHistogram & JSKeyedHistogram functions per STR-1?

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8835536 [details] [diff] [review]
bug1321349.patch

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

Lets try to get test coverage for this working.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: bug1321349.patch

Thanks for catching that, Georg. I've updated the patch to fix Clear() too (for both plain and keyed scalars), and slightly changed the .dataset() function as well.

I've added test coverage for the .add()/.clear() cases.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8839077 [details] [diff] [review]
bug1321349.patch

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

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1571,4 @@
>    if (args.length() >= 1) {
>      if (!args[0].isBoolean()) {
> +      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Not a boolean"));
> +      return true;

This function should only be called from tests.
I think its fair to leave it as throwing an error (and call it out as test-only in nsITelemetry.idl).
Ditto for the keyed version.

@@ +1608,5 @@
> +  // This function should always return |undefined| and never fail but
> +  // rather report failures using the console.
> +  args.rval().setUndefined();
> +  LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid histogram ID."));
> +  return true;

This can throw a JS error, it should only be called from Telemetry test code.
We should just drop the dataset() functions from keyed & plain histograms (and test via the nsITelemetry snapshot functions instead).
Can you file a follow-up?

@@ +1897,5 @@
>    if (NS_FAILED(rv)) {
> +    // This function should always return |undefined| and never fail but
> +    // rather report failures using the console.
> +    args.rval().setUndefined();
> +    // According to the GetDataset, we should never get here.

So lets just make GetDataset() infallible and directly return the value.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ +290,5 @@
> +  let h = Telemetry.getHistogramById("TELEMETRY_TEST_LINEAR");
> +  Assert.strictEqual(h.clear(), undefined,
> +                     "The clear() function must return undefined.");
> +  Assert.strictEqual(h.add(1), undefined,
> +                     "The add() function must return undefined.");

It would be good to consistently test all functions:
"add", "clear", "snapshot", "dataset"
... plus for keyed histograms:
"subsessionSnapshot", "snapshotSubsessionAndClear", "keys"

Maybe:
> for (let fn of [h.add(1), ...])
>   Assert.strictEqual(fn(), undefined, ...);
> // Ditto for the .apply version.

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: bug1321349.patch

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Comment on attachment 8839077 [details] [diff] [review]
> bug1321349.patch
> 
> Review of attachment 8839077 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1608,5 @@
> > +  // This function should always return |undefined| and never fail but
> > +  // rather report failures using the console.
> > +  args.rval().setUndefined();
> > +  LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid histogram ID."));
> > +  return true;
> 
> This can throw a JS error, it should only be called from Telemetry test code.
> We should just drop the dataset() functions from keyed & plain histograms
> (and test via the nsITelemetry snapshot functions instead).
> Can you file a follow-up?

Filed bug 1341236.

> ::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
> @@ +290,5 @@
> > +  let h = Telemetry.getHistogramById("TELEMETRY_TEST_LINEAR");
> > +  Assert.strictEqual(h.clear(), undefined,
> > +                     "The clear() function must return undefined.");
> > +  Assert.strictEqual(h.add(1), undefined,
> > +                     "The add() function must return undefined.");
> 
> It would be good to consistently test all functions:
> "add", "clear", "snapshot", "dataset"
> ... plus for keyed histograms:
> "subsessionSnapshot", "snapshotSubsessionAndClear", "keys"
> 
> Maybe:
> > for (let fn of [h.add(1), ...])
> >   Assert.strictEqual(fn(), undefined, ...);
> > // Ditto for the .apply version.

All the other functions (except add and clear) return objects or arrays (e.g. snapshot() returns the current data snapshot).

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> (In reply to Alessio Placitelli [:Dexter] from comment #3)
> > This fixes the STR-2, which can be reproduced with:
> ...
> > However, when trying to add test coverage, something funny came up. I added
> > the following lines after [1], and the test would just "lock up". It doesn't
> > crash (as expected) but, for some reason, it just never completes and times
> > out.
> 
> Is it only timing out without the patch?
> If so, we can still add this as test coverage.

What happened to this?

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8839397 [details] [diff] [review]
bug1321349.patch

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

This looks good, but still misses coverage for STR-2.
Is this coming in a separate patch here?

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +873,5 @@
>                   uint32_t bucketCount, uint32_t dataset);
>    nsresult GetHistogram(const nsCString& name, Histogram** histogram, bool subsession);
>    Histogram* GetHistogram(const nsCString& name, bool subsession);
>    uint32_t GetHistogramType() const { return mHistogramType; }
> +  uint32_t GetDataset() const;

I assume the dataset functions are now removed anyway, after re-basing.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ +294,5 @@
> +    hist.clear(),
> +    hist.add(1),
> +    keyedHist.clear(),
> +    keyedHist.add("some-key", 1),
> +

Remove empty line.

@@ +299,5 @@
> +  ];
> +
> +  for (let returnValue of RETURN_VALUES) {
> +    Assert.strictEqual(returnValue, undefined,
> +                       "The function function must return undefined.");

"function function"

Comment 13

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] [away March 24 - April 2] from comment #11)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> > (In reply to Alessio Placitelli [:Dexter] from comment #3)
> > > This fixes the STR-2, which can be reproduced with:
> > ...
> > > However, when trying to add test coverage, something funny came up. I added
> > > the following lines after [1], and the test would just "lock up". It doesn't
> > > crash (as expected) but, for some reason, it just never completes and times
> > > out.
> > 
> > Is it only timing out without the patch?
> > If so, we can still add this as test coverage.
> 
> What happened to this?

As discussed, this will be taken care of in bug 1341084.

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: bug1321349.patch

Comment 15

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ecbef40e414

Comment 16

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcada7b131f44a38ea6836da58ebc1b9f547637
Bug 1321349 - Make histogram.add check 'this' for the proper JSClass. r=gfritzsche

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcada7b131f
Make histogram.add check 'this' for the proper JSClass. r=gfritzsche

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/4bcada7b131f