Closed Bug 1045801 Opened 5 years ago Closed 5 years ago

Rename SafeCast to AssertedCast

Categories

(Core :: MFBT, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bjacob, Assigned: poiru)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

SafeCast just adds a debug-only assert that the input value is in bounds. In release builds, it doesn't check anything, contrary to what its name implies.

So if people start relying on SafeCast instead of CheckedInt to validate inputs then they're not going to get the safety that they expect.

Integer overflow conditions are typically hit 1) in the real world and 2) in fuzzing, but not so much 3) in our own regular testing, meaning that they tend to be hit in release builds not in debug builds -- outside of fuzzing. But while we do have some fuzzing, we don't have enough of it to feel comfortable relying on it to hit all integer overflow conditions in assertions such as provided by SafeCast.
SafeCast behaves this way *by design*.  It is not a replacement for CheckedInt.  It is a way to convert a value from one type to another, when it's known the value will fit in the target type.  If you want to bikeshed the name, that's fine enough, but its behavior isn't going to change.

I don't actually think people are likely to use SafeCast when they really want CheckedInt, to be honest.  The Checked name is pretty idiomatic, and I don't see people reaching for one and finding the other all that often.
Oh, the name was imported from WebKit's WTF::safeCast.  I'm not aware of their having had problems with people misusing safeCast when they really wanted Checked.
For example, here,

http://dxr.mozilla.org/mozilla-central/source/mfbt/decimal/moz-decimal-utils.h#57

this function gets an arbitrary std::string, so its length can be any size_t. How is it the case that "it's known the value will fit in the target type" ?
I believe that's because mozToDouble is only used inside Decimal.cpp as shim code between the original Decimal code, and our particular needs for it, and the input is the toString() of a Decimal object, and Decimal doesn't generate unbounded strings for that method.  See also line 11 of that file.  Put simply, that function doesn't get arbitrary std::string.
Depends on: 1045829
OK, filed bug 1045829 about that one.
Can we call it CastWithDebugBoundsChecks?  I just caught a piece of code which was being misled by the name "Safe"Cast.
Flags: needinfo?(jwalden+bmo)
Talking to Waldo on IRC, I suggested AssertedCast which is one character longer than static_cast, and Waldo didn't seem to run away in total disgust.
CWDBC is way too long, for sure, for a function that we really want to be used every place a cast occurs.  (Excepting casts to unsigned type where wraparound is desired.)  AssertedCast is still a little screwy but close enough to not be completely horrible.
Flags: needinfo?(jwalden+bmo)
poiru, is this something that interests you?  Fortunately we don't have too many SafeCast consumers yet!
Flags: needinfo?(birunthan)
Summary: SafeCast doesn't give any safety in release builds → Rename SafeCast to AssertedCast
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> poiru, is this something that interests you?  Fortunately we don't have too
> many SafeCast consumers yet!

Sure.
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #8474826 - Flags: review?(ehsan)
Flags: needinfo?(birunthan)
Comment on attachment 8474826 [details] [diff] [review]
Rename SafeCast to AssertedCast

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

I'll let Waldo r+ this to confirm that he's fine with the rename.  The patch itself is trivial though.

Thanks!
Attachment #8474826 - Flags: review?(ehsan) → review?(jwalden+bmo)
Comment on attachment 8474826 [details] [diff] [review]
Rename SafeCast to AssertedCast

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

::: js/src/builtin/TypedObject.cpp
@@ +28,5 @@
>  #include "vm/Shape-inl.h"
>  
>  using mozilla::CheckedInt32;
>  using mozilla::DebugOnly;
> +using mozilla::AssertedCast;

Realphabetize this.

::: js/src/jit/IonBuilder.cpp
@@ +34,5 @@
>  using namespace js::jit;
>  
>  using mozilla::DebugOnly;
>  using mozilla::Maybe;
> +using mozilla::AssertedCast;

Realphabetize.

::: js/src/jsstr.cpp
@@ +65,5 @@
>  using mozilla::Move;
>  using mozilla::PodCopy;
>  using mozilla::PodEqual;
>  using mozilla::RangedPtr;
> +using mozilla::AssertedCast;

Realphabetize.
Attachment #8474826 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/e8e520884505
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.