1947140 - A bug in JIT optimization: an exception about '>=' operator

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[anbu]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/132.0.0.0 Safari/537.36 Edg/132.0.0.0

Steps to reproduce:

A bug in JIT optimization, whether there are security risks or not requires further in-depth analysis.

version:commit 3166120f85161b7fe84565135b04c685da1dd34f

Build options:

/bin/sh ../../gecko-dev/js/src/configure --enable-debug --disable-optimize --disable-shared-js --disable-tests --enable-gczeal

Test case:

./js --baseline-warmup-threshold=10 --ion-warmup-threshold=50 --ion-check-range-analysis --ion-extra-checks

case1:

function opt(){
  const v0 = class v0 extends Uint8ClampedArray {
      constructor() {
      }
      concat() {
      }
      asinh() {
      }
  };
  const v1 = Reflect.parse(v0);
  const v2 = v0.toLocaleString();
  const v3 = "E" + v2;
  const v4 = "E" >= v3;
  return v4;
}
  
let a = opt();
let b = opt();
for (let i = 0; i < 55; i++) {
  opt();
}
let c = opt();
print(a);
print(b);
print(c);

case2:

function opt(){
  const v0 = [];
  v0[255] = v0;
  function v1() {
  }
  const v2 = new v1(...v0);
  const v3 = "9".concat(BigInt);
  const v4 = v3 <= "9";
  return v4;
}
  
let a = opt();
let b = opt();
for (let i = 0; i < 55; i++) {
  opt();
}
let c = opt();
print(a);
print(b);
print(c);

case3:

function opt(opt_param){
  const v0 = Object.getOwnPropertyDescriptors(Object);
  const v1 = Object.getOwnPropertyNames(v0);
  const v2 = Float32Array.from(v1);
  const v3 = "9".concat(v2);
  const v4 = v3 <= "9";
  return v4;
}
  
let a = opt();
let b = opt();
for (let i = 0; i < 55; i++) {
  opt();
}
let c = opt();
print(a);
print(b);
print(c);

Actual results:

The result has changed after JIT optimization.
case1:

a = false
b = false
c = true

case2:

a = false
b = false
c = true

case3:

a = false
b = false
c = true

Comment 1

[Iain Ireland [:iain]]

This is a good find. It appears to be a bug in CodeGenerator::visitCompareSSingle. Here's a reduced testcase:

// |jit-test| --ion-inlining=off; --fast-warmup; --ion-offthread-compile=off
function foo(x) { return "a sufficiently long string"; }
function opt(){
  return "E" >= "E" + foo();
}

for (let i = 0; i < 55; i++) {
  assertEq(opt(), false);
}

We're comparing the constant string "E" to the rope "E" + "a sufficiently long string" (where the length of the right child just needs to be long enough to prevent us from creating an inline string). We find the leftmost child of the rope and compare it to the constant. They compare equal, so we fall back to comparing lengths. However, we're accidentally comparing the length of the leftmost child, not the entire rope. If instead of using temp as the base register for loading the length, we use input, then all three testcases are fixed.

This is only a correctness issue, not a security issue. We get the wrong answer, but not in any sort of exploitable way.

Comment 2

[Iain Ireland [:iain]]

Attachment: Bug 1947140: Compare against correct length in visitCompareSSingle r=jandem

Comment 3

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1861926

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90c2d0ea11ed
Compare against correct length in visitCompareSSingle r=jandem

Comment 5

[amarc]

https://hg.mozilla.org/mozilla-central/rev/90c2d0ea11ed

Comment 6

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:iain, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox136 to wontfix.

For more information, please visit BugBot documentation.

Comment 7

[Iain Ireland [:iain]]

Comment on attachment 9465300 [details]
Bug 1947140: Compare against correct length in visitCompareSSingle r=jandem

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Potential for incorrect results in code that could realistically occur in the wild.
• 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): It's a one-line change that has no effect in non-buggy cases.
• String changes made/needed: None
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Potential for incorrect results in code that could realistically occur in the wild.
• User impact if declined: Certain strings don't compare properly.
• Fix Landed on Version: 137
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a one-line change that has no effect in non-buggy cases.

Comment 8

[Donal Meehan [:dmeehan]]

Comment on attachment 9465300 [details]
Bug 1947140: Compare against correct length in visitCompareSSingle r=jandem

Approved for 136.0b6

Comment 9

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/d775d12409d2

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9465300 [details]
Bug 1947140: Compare against correct length in visitCompareSSingle r=jandem

Approved for 128.8esr

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/fec278710b4b