All users were logged out of Bugzilla on October 13th, 2018

Signed integer overflow in js::AddOperation

RESOLVED FIXED in mozilla27

Status

()

--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks: 1 bug, {sec-want})

Trunk
mozilla27
x86_64
Linux
sec-want
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [-fsanitize=signed-integer-overflow])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
The following code in js/src/jspipelines.h performs unchecked addition of two signed integers (which can actually be large enough to overflow):

> static JS_ALWAYS_INLINE bool
> AddOperation(JSContext *cx, HandleScript script, jsbytecode *pc, 
> const Value &lhs, const Value &rhs, Value *res)
> {
>     if (lhs.isInt32() && rhs.isInt32()) {
>         int32_t l = lhs.toInt32(), r = rhs.toInt32();
>         int32_t sum = l + r;


This won't lead to an immediate failure here, but signed integer overflowing is not defined according to the C standard and should not be used (INT32-C, CERT Secure Coding Standard).

jandem suggested earlier to use

> int32_t sum = uint32_t(l) + uint32_t(r);

instead but also mentioned it could be that SubOperation exhibits the same problem. That method however seems to be using doubles instead of integers, and then seems to check for NaN as result of an overflow.

Updated

5 years ago
Whiteboard: [-fsanitize=signed-integer-overflow]
(Reporter)

Comment 1

5 years ago
Needinfo from Jandem (if you're not the right person, please forward to someone else, e.g. Waldo): We're trying to get rid of signed integer overflows. They are not only bad in theory, but GCC can also perform bad optimizations based on such overflows just by assuming they don't happen.

Is it possible to get a fix here? Maybe the one suggested by jandem?
Flags: needinfo?(jdemooij)

Updated

5 years ago
Blocks: 919486
(Assignee)

Comment 2

5 years ago
Created attachment 810534 [details] [diff] [review]
Patch

This patch just uses double addition + setNumber. It looks simpler and with the JITs this is not performance critical. SubOperation already uses double arithmetic.

(Bug 915763 will remove the MonitorOverflow call completely.)
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #810534 - Flags: review?(bhackett1024)
Flags: needinfo?(jdemooij)
Attachment #810534 - Flags: review?(bhackett1024) → review+
If it's not performance critical we could change it to:

if (isNumber && isNumber) { toNumber () + toNumber() }
https://hg.mozilla.org/mozilla-central/rev/14ad832ecbcd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.