1352261 - Consider making |CheckedInt::value| release assert |isValid|

Status: RESOLVED DUPLICATE of bug 1533777

(Core :: MFBT, enhancement)

Description

[Eric Rahm [:erahm]]

We should make the assertion in |CheckedInt::value| [1] a release assertion. The general assumption is that by switching to using CheckedInt you get a validity check "for free".

For example I just wrote some coding doing essentially:

> auto byteLen = (CheckedInt<size_t>(aLen) + 1) * sizeof(char_type);
> void* p = malloc(byteLen.value());

I expected that to prevent passing the wrong size to |malloc| by crashing safely instead.

[1] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/mfbt/CheckedInt.h#559

Comment 1

[Jeff Walden [:Waldo]]

(In reply to Eric Rahm [:erahm] from comment #0)
> The general assumption is that by switching to using CheckedInt you get a
> validity check "for free".

Why is that so?  I don't think this is at all obvious, particularly when the value() function as a matter of signature can't cleanly handle errors.  The only theoretical way the value() function might "handle" things here is to crash, but it's far from the normal case that mathematical miscomputation should result in a crash.  It doesn't seem to me that behavior and performance should be pessimized for that rare case.

Comment 2

[Eric Rahm [:erahm]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> (In reply to Eric Rahm [:erahm] from comment #0)
> > The general assumption is that by switching to using CheckedInt you get a
> > validity check "for free".
> 
> Why is that so?  I don't think this is at all obvious, particularly when the
> value() function as a matter of signature can't cleanly handle errors. 

We already crash in debug, so we already made this expected behavior.

> The
> only theoretical way the value() function might "handle" things here is to
> crash, but it's far from the normal case that mathematical miscomputation
> should result in a crash.

I'd argue it should, and I have seen plenty of places where we'd *much* rather it did.

> It doesn't seem to me that behavior and
> performance should be pessimized for that rare case.

The only reason I'm going to use CheckedInt is if I care about safety from an integer overflow, perf is less of a concern. If I didn't care about safety I wouldn't use CheckedInt.

I'd rather have crashing be opt-out by checking |isValid| and bailing rather than encouraging accidental usage of a !isValid value.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to Eric Rahm [:erahm] from comment #2)
> We already crash in debug, so we already made this expected behavior.

Debug build behavior is not a safety net, nor is it a guarantee of anything.  It's assistance during debugging.  Same, it's worth noting, as Rust's integer overflow behavior.

> The only reason I'm going to use CheckedInt is if I care about safety from
> an integer overflow, perf is less of a concern. If I didn't care about
> safety I wouldn't use CheckedInt.

CheckedInt isn't specifically for safety: it's for performing computations that might go astray and letting the user detect if that happens.  Safety is a function of how the CheckedInt is used.

That said, I think these days we could reasonably convert CheckedInt to return Result<T>.  That would compel users to check for validity before accessing the inner value -- or at least if users didn't it would be even more clear that it was their fault for not doing so.