If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

TM: new array with negative array length is not handled properly on trace

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P2
major
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Franck, Assigned: gal)

Tracking

Trunk
x86
Windows XP
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 final-fixed, status1.9.1 unaffected)

Details

(Whiteboard: [sg:dos], fixed-in-tracemonkey)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Using the latest tip (2009.11.23), the following script hang with 100% CPU when JIT is enabled:

for ( var i = 0; i < 10; i += 0.1 ) {
  
  var len = Math.round(Math.sin(i));
  new Array(len).join('*');
}

(in doubt, I checked the security checkbox)
(Reporter)

Updated

8 years ago
Summary: scripts can lock the JavaScript engine when JSI is enabled → scripts can lock the JavaScript engine when JIT is enabled
(Reporter)

Comment 1

8 years ago
My OS: win32
Let's let others look at the bug before marking it sensitive.

/be
Group: core-security
(Reporter)

Comment 3

8 years ago
Simpler testcase:

for ( var i = 1; i > -2; i-- )
  new Array(i).join('*');
(Assignee)

Updated

8 years ago
Assignee: general → gal
Assignee: gal → general
Assignee: general → gal
Comment 3 much appreciated; looks like something's going wrong when we JIT new Array(-1) -- it should throw, it doesn't, join() works on a 2**32-1 length array and "hangs".
(Assignee)

Updated

8 years ago
Group: core-security
(Assignee)

Comment 5

8 years ago
Not hiding as per brendan.

The jit is making an empty array and then we generate LIR to write in the length but don't check for < 0.
Group: core-security
(Assignee)

Comment 6

8 years ago
Created attachment 414127 [details] [diff] [review]
patch
Attachment #414127 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

8 years ago
Created attachment 414130 [details] [diff] [review]
patch
Attachment #414127 - Attachment is obsolete: true
Attachment #414130 - Flags: review?(jwalden+bmo)
Attachment #414127 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

8 years ago
Btw, great testcase Franck. Thanks. Was trivial to reproduce.
Attachment #414130 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 414130 [details] [diff] [review]
patch

r+ with some trace tests for this
(Assignee)

Updated

8 years ago
Flags: blocking1.9.2?
Priority: -- → P2
Summary: scripts can lock the JavaScript engine when JIT is enabled → TM: new array with negative array length is not handled properly on trace
(Assignee)

Comment 10

8 years ago
"Prudent to take." to quote Brendan. Small patch and we can test it well.

http://hg.mozilla.org/tracemonkey/rev/9f3d49780173
Whiteboard: [sg:dos], fixed-in-tracemonkey

Updated

8 years ago
Flags: blocking1.9.2? → blocking1.9.2+

Comment 11

8 years ago
http://hg.mozilla.org/mozilla-central/rev/9f3d49780173
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Created attachment 415447 [details] [diff] [review]
Tabs removal

The patch has introduced tabs in the test case. This patch corrects this.
Attachment #415447 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 13

8 years ago
Comment on attachment 415447 [details] [diff] [review]
Tabs removal

Stealing. I think strictly speaking this is NPOTB (the tab) so no review is necessary (IMO).
Attachment #415447 - Flags: review?(jwalden+bmo) → review+
Indeed.  Whitespace fixes, spelling fixes, and other trivial nits not affecting execution of code have always been exempt from needing review -- not worth having someone spend time on it beyond just landing the fix.

Comment 15

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4ff4b635baa9
status1.9.2: --- → final-fixed
status1.9.1: --- → unaffected
You need to log in before you can comment on or make changes to this bug.