811264 - Signed integer overflows in js::Int32ToString and IntToCString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Patch

There are two signed integer overflows in the mentioned functions (in js/src/jsnum.cpp), when trying to get the absolute positive uint from a (checked) negative int:

> js::Int32ToString(JSContext *cx, int32_t si)
> {
>     uint32_t ui;
>     if (si >= 0) {
>         if (StaticStrings::hasInt(si))
>             return cx->runtime->staticStrings.getInt(si);
>         ui = si;
>     } else {
>         ui = uint32_t(-si); // <<<---- HERE
>         JS_ASSERT_IF(si == INT32_MIN, ui == uint32_t(INT32_MAX) + 1);
>     }

and 

> static char *
> IntToCString(ToCStringBuf *cbuf, int i, int base = 10)
> {
>    unsigned u = (i < 0) ? -i : i; <<<---- HERE


This won't lead to immediate failures because the compiler will usually do what one expects but this isn't guaranteed. We should explicitly use the proper casts to avoid signed integer overflowing. (INT32-C, CERT Secure Coding Standard).

Attached is a patch. I'm confident about the first change, but not exactly sure if the second is correct on all platforms (but I guess it is). Feedback welcome.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 681000 [details] [diff] [review]
Patch

MSVC will warn if you apply unary minus to an unsigned type.

Could we switch this to uint32_t instead of int32_t, maybe?  My recollection is our fast-path behavior only works for small unsigned numbers anyway.

Comment 2

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> MSVC will warn if you apply unary minus to an unsigned type.

What do you recommend then? It seems like the right fix to me.

I think the hottest path into Int32ToString is from js::ToStringSlow, which obviously needs the whole int32 range covered.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 681000 [details] [diff] [review]
Patch

I mean, we could certainly change Int32ToString to take a uint32_t, and then it wouldn't need to do this conversion. But we would only have moved the problem to js::ToStringSlow. And in any case COME ON we're talking about an operation that maps exactly to a single arithmetic instruction, C's exact target problem domain. Are we mice or are we hackers?

I say add the `#ifdef _MSC_VER #pragma warnings push` weaksauce in this .cpp file. The warning is dumb.

Clearing review flag for now.

(Oh - we should probably change IntToCString to Int32ToCString though.)

Comment 4

[Jeff Walden [:Waldo]]

Oh, right, this is needed for ToStringSlow.  So of course this has to be Int32ToString and Int32ToCString (renaming's clearly the right thing to do).

What we should do is move NegateNegativeInt32(int32_t i) out of jsarray.cpp and use that.  For lack of a better place, maybe jsnum.h for now?

Comment 5

[Christian Holler (:decoder)]

Waldo, can we get this patch in, or does it need changes? Thanks!

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Use mozilla::Abs instead of open-coding in warning-inducing ways

Oh hey, we have mozilla::Abs to eliminate all the crazy.  (And plus the rename mentioned earlier.)

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 827644 [details] [diff] [review]
Use mozilla::Abs instead of open-coding in warning-inducing ways

Review of attachment 827644 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. And I'm sorry for vanishing for two weeks.

Comment 8

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b93dd3c97b5

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b93dd3c97b5