Closed Bug 584158 Opened 12 years ago Closed 12 years ago
ensure that typed arrays cannot produce non-canonical nans
Typed arrays let users construct doubles from bits by punning between int8s and doubles. With fatvals, we use non-canonical nans to encode all the other types a jsval may contain. Thus, we must guard, whenever reading a double from a typed array, that, if it holds a nan, it holds a canonical nan. Otherwise, things will go poorly. Talking to vlad, it seems there are there are at least two access paths we need to protect: reading a double slot on trace and reading a double slot through the typed array's getter.
This fix should also block fatvals release into a beta.
Typed arrays were meant to solve a problem mapping C structs and arrays of structs (arrays of structs containing array members, etc.) into JS, but the aliasing ability was not meant to be used to pun storage. Argh! /be
Is this a security bug? Can we forge values through this path? If not, please re-open.
Sure can, you can write whatever bits you want in and then read it as a double. This wasn't a (big) issue before, but isn't great now. Luke and I think that just adding some canonicalization on read wouldn't be too expensive.
blocking2.0: --- → ?
Whiteboard: [sg:investigate?] → [sg:critical]
let's try to get this fixed ASAP, and get it in the beta
Whiteboard: [sg:critical] → [sg:critical][critsmash:investigating]
Here's the basic fix (which requires LIR_cmovd in bug 584214). Also need to write some tests that exercise this path.
While I'm waiting on cmovd, this version uses a silly insAlloc'd phi node. The attached test is confirmed to crash without the changes and is fixed with the changes. The question is whether I have caught all the ways that a punned double can be read from a typed array. vlad could you double check me and just ignore TraceRecorder::canonicalizeNaNs?
Yeah, pretty sure you got them all -- but please please don't check this in without cmovd, it will likely be brutal for performance. I'll get some benchmarks up to see what happens.
cmovd just went in, so I'm about to post a patch with that.
Comment on attachment 462667 [details] [diff] [review] fix with cmovd Always set cmov flag to true for lircmovd. Please test in fpu mode.
Attachment #462667 - Flags: review?(gal) → review+
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:investigating] fixed-in-tracemonkey
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.