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

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: benjamin, Assigned: Dexter)

Tracking

(Blocks 2 bugs)

unspecified
mozilla55
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment, 3 obsolete attachments)

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.
Thanks for the detailed report!
Priority: -- → P2
Whiteboard: [measurement:client]
(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.
Priority: P2 → P1
Points: --- → 1
Priority: P1 → P2
Priority: P2 → P1
Assignee: nobody → alessio.placitelli
Posted patch bug1321349.patch (obsolete) — Splinter Review
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
Attachment #8835536 - Flags: review?(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.
(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 on attachment 8835536 [details] [diff] [review]
bug1321349.patch

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

Lets try to get test coverage for this working.
Attachment #8835536 - Flags: review?(gfritzsche)
Posted patch bug1321349.patch (obsolete) — Splinter Review
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.
Attachment #8835536 - Attachment is obsolete: true
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.
Attachment #8839077 - Flags: feedback+
Blocks: 1341084
Posted patch bug1321349.patch (obsolete) — Splinter Review
Attachment #8839077 - Attachment is obsolete: true
Attachment #8839397 - Flags: review?(gfritzsche)
(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).
(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?
Flags: needinfo?(alessio.placitelli)
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"
Attachment #8839397 - Flags: review?(gfritzsche) → review+
(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.
Flags: needinfo?(alessio.placitelli)
Attachment #8839397 - Attachment is obsolete: true
Attachment #8851508 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcada7b131f44a38ea6836da58ebc1b9f547637
Bug 1321349 - Make histogram.add check 'this' for the proper JSClass. r=gfritzsche
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcada7b131f
Make histogram.add check 'this' for the proper JSClass. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/4bcada7b131f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.