Christian Holler (:decoder) Reporter
	Description • 12 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.

	Christian Holler (:decoder) Reporter
	Comment 1 • 11 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?

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 11 years ago

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.)

	Jan de Mooij [:jandem] Assignee
	Comment 3 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/14ad832ecbcd

	Tom S [:evilpie]
	Comment 4 • 11 years ago

If it's not performance critical we could change it to:

if (isNumber && isNumber) { toNumber () + toNumber() }

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/14ad832ecbcd