CheckedInt.h depends on undefined value of signed arithmetic

RESOLVED FIXED in mozilla15

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla15
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 627798 [details] [diff] [review]
Use unsigned operations

This was found by clang:

https://tbpl.mozilla.org/php/getParsedLog.php?id=12136397&tree=Try&full=1#error0

When trying to find out if a+b or a-b is valid, CheckedInt.h would use the computed result of a+b (or a-b), but that value is undefined for signed types.
Assignee: nobody → respindola
Attachment #627798 - Flags: review?(bjacob)
Attachment #627798 - Attachment is patch: true
Comment on attachment 627798 [details] [diff] [review]
Use unsigned operations

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

::: mfbt/CheckedInt.h
@@ +319,4 @@
>  
>  template<typename T>
>  inline bool
> +IsAddValid(T x, T y)

Passing the result was an optimization, but a minor and premature one, and this is expensive enough to make the cost of this operation negligible.

@@ +328,5 @@
>    // if T was a small type like int8_t, so we explicitly cast to T.
> +
> +  typename UnsignedType<T>::Type ux = x;
> +  typename UnsignedType<T>::Type uy = y;
> +  typename UnsignedType<T>::Type result = ux + uy;

If MSVC gives you compiler errors with that, try making a typedef:
typename UnsignedType<T>::Type UType;
UType ux = x;
Attachment #627798 - Flags: review?(bjacob) → review+
Comment on attachment 627798 [details] [diff] [review]
Use unsigned operations

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

::: mfbt/CheckedInt.h
@@ +680,4 @@
>    T y = rhs.mValue;                                                   \
>    T result = x OP y;                                                  \
>    T isOpValid                                                         \
> +      = detail::Is##NAME##Valid(x, y);                        \

Line up the \. (This can become one line now, I think.)
I have pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=93040f4eb3e0

and will push to mozilla-inbound if that is green.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=98410387837c

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/98410387837c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.