mfbt rotate functions have undefined behavior when rotating by 0

RESOLVED DUPLICATE of bug 939157

Status

()

RESOLVED DUPLICATE of bug 939157
3 years ago
3 years ago

People

(Reporter: sunfish, Unassigned)

Tracking

({regression})

Trunk
regression
Points:
---

Firefox Tracking Flags

(firefox41 affected, firefox42 affected, firefox43 affected, firefox44 affected, firefox45 affected, firefox-esr38 affected)

Details

(Whiteboard: [fuzzblocker])

(Reporter)

Description

3 years ago
MFTB's RotateLeft in mfbt/MathAlgorithms.h does this:

  return (aValue << aShift) | (aValue >> (sizeof(T) * CHAR_BIT - aShift));

When aShift is 0, the shift count for the right shift is the bitwidth of T, which invokes undefined behavior.

RotateRight in the same file also has this bug.

A relatively cheap way to fix this would be to mask aShift with (sizeof(T) * CHAR_BIT - 1) first.
We might want to consolidate all of our discussions / observations of these bugs:

Bug 939157 -  RotateLeft/RotateRight has undefined behavior for shift == 0

I had originally hit this through:

Bug 1220275 - Differential Testing: Different output message involving arrays on ARM-simulator builds
Bug 1220915 -  Crash [@ js::CompartmentChecker::check]

both of which seemed to be specific to ARM-simulator builds on Mac, which uses Clang. I was unable to get a working build of the js shell compiled via GCC on Mac.
Group: javascript-core-security
Group: javascript-core-security
This affects every version since (presumably) the original code landed in 2013:

https://hg.mozilla.org/mozilla-central/rev/7c3792d389bc

and this blocks fuzzing using Mac ARM-simulator builds, so setting [fuzzblocker].

Any takers?
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox-esr38: --- → affected
Keywords: regression
Whiteboard: [fuzzblocker]
Version: unspecified → Trunk
This (essentially a dupe of bug 939157) should be opened up because bug 939157 is open and has a landed patch.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 939157
Group: core-security
You need to log in before you can comment on or make changes to this bug.