Closed Bug 1650526 Opened 4 years ago Closed 4 years ago

Issue involving negative zero and Math.pow

Categories

(Core :: JavaScript Engine: JIT, defect)

All
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox77 --- unaffected
firefox78 --- unaffected
firefox79 - fixed
firefox80 - fixed

People

(Reporter: gkw, Assigned: anba)

References

(Regression)

Details

(Keywords: regression, testcase)

Attachments

(1 file)

let x = [];
let y = [2147483648, 2147483649];
for (let i = 0; i < 2; ++i) {
    x.push(Math.pow(-2147483648, -y[i]));
}
assertEq(uneval(x[1]), "-0");

AR=ar sh ./configure --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Assertion fails on latest m-c rev efa2336315eda0aaf78c2e94d6c9c98106ea136b with --fuzzing-safe --no-threads --ion-eager, using clang 9.0.1, passes when run with --fuzzing-safe --no-threads --baseline-eager --no-ion. Also passes when run on rev 5b9343de266b3a4b7fa585345027b6589ea4614f, the parent of the potential regressor on both sets of flags.

$ ./js-dbg-64-dm-linux-x86_64-efa2336315ed --fuzzing-safe --no-threads --ion-eager testcase-assertEq.js 
testcase-assertEq.js:18:9 Error: Assertion failed: got "0", expected "-0"
Stack:
  @testcase-assertEq.js:18:0
The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/bfc836af8677
user:        André Bargull
date:        Wed Jun 10 11:51:58 2020 +0000
summary:     Bug 1564942 - Part 2: Avoid negative zero check when the base operand in MPow is an Int32. r=jandem

Unsure how bad this is, so marking s-s and setting needinfo? from :anba and :jandem. Please open up if this is not the case.

Flags: sec-bounty?
Flags: needinfo?(jdemooij)
Flags: needinfo?(andrebargull)

Thanks for reporting, Gary! I'll leave the analysis to anba.

Flags: needinfo?(jdemooij)

I don't think this can be exploited in any way, because we're only losing the sign, but all other parts around MPow and MToNumberInt32 are still working correctly. And in the past we also didn't hide +0/-0 bugs, so it doesn't look like we're aware of any issues which can make this exploitable: bug 1308802, bug 1312620, bug 1314438.

Flags: needinfo?(andrebargull)
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED

Removes the incorrect optimisation added in D37584 for IonBuilder and keeps it
only in MPow::foldsTo() when optimising the cases when power ∈ {2,3,4}.
Also changes MPow::canBeNegativeZero()'s access modifier to private, now that
it's only used within MPow::foldsTo().

I'll request uplift, because the fix is easy and safe to do.

(In reply to Julien Cristau [:jcristau] from comment #3)

Not tracking based on comment 2.

Note that it's still a correctness bug though. Not a very interesting one but I thought we still wanted to track those.

Group: core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

Comment on attachment 9161556 [details]
Bug 1650526: Preserve negative zero for MPow with int32 specialisation. r=jandem!

Beta/Release Uplift Approval Request

  • User impact if declined: The Math.pow built-in can return positive zero (+0) instead of negative zero (-0) in certain cases.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Not risky because it only removes an incorrect optimisation added in bug 1564942, so we're back to what we had before bug 1564942.
  • String changes made/needed:
Attachment #9161556 - Flags: approval-mozilla-beta?

Comment on attachment 9161556 [details]
Bug 1650526: Preserve negative zero for MPow with int32 specialisation. r=jandem!

Approved for 79.0b6.

Attachment #9161556 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: core-security-release
Flags: sec-bounty? → sec-bounty-
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: