849666 - CheckedInt unary operator- guilty of undefined behavior

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fix undefined behavior in CheckedInt unary operator-

-minvalue overflows, which is undefined behavior for signed types. This also allows to remove OppositeIfSignedImpl... hopefully warnings won't be back, but that wouldn't be the end of the world.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 723268 [details] [diff] [review]
fix undefined behavior in CheckedInt unary operator-

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

I mentioned this to Jesse recently, and he was surprised there'd even be an operator-() on unsigned CheckedInts.  I think there's a pretty good case that we should remove the operator for unsigned types.  But this at least fixes the undefined behavior, so this is a step forward.  We can remove operator-() after this immediate issue's fixed.

Comment 2

[Jeff Walden [:Waldo]]

Actually, hmm, yes, this does have the warning issue still -- should have read bugmail more closely.  Thinking about that now.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 723268 [details] [diff] [review]
fix undefined behavior in CheckedInt unary operator-

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

::: mfbt/CheckedInt.h
@@ +608,5 @@
>      {
> +      if (!detail::IsSubValid(T(0), mValue))
> +        return CheckedInt(0, false);
> +
> +      return CheckedInt(-mValue, mIsValid);

if (IsSigned<T>::value) {
  if (mValue == MinValue<T>::value)
    return CheckedInt(mValue, false);
  return CheckedInt(-mValue, true);
}

return CheckedInt(mValue, mValue == 0);

What about this instead, to avoid the warning?

Comment 4

[Jeff Walden [:Waldo]]

Wait, I fail, that doesn't avoid the warning either.  Lemme think.

Comment 5

[Jeff Walden [:Waldo]]

How about these inside of CheckedInt, in a protected section:

template<typename T, bool IsSigned = IsSigned<T>::value>
static CheckedInt<T>
Negate(const CheckedInt<T>& val)
{
  return CheckedInt(0, val.isValid() && val.mValue == 0);
}

template<typename T, true>
static CheckedInt<T>
Negate(const CheckedInt<T>& val)
{
  if (!val.isValid() || val.mValue == MinValue<T>::value)
    return CheckedInt(val.mValue, false);
  return CheckedInt(-val.mValue, true);
}

CheckedInt operator -() const
{
  return Negate(*this);
}

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Another try, validates as having no UB with -fsanitize=undefined, shouldn't

Okay, this actually avoids the - warning and validates against -fsanitize=undefined.  The friend-ness is slightly sad, but you can't partially specialize function templates (at least not without C++11, maybe), so a separate class is needed (to access CheckedInt(T, bool), to be clear).  I could use an inner class of CheckedInt, but I think even that might need friend access but it seems nicer keeping it out of the interface bits.

Comment 7

[Jeff Walden [:Waldo]]

Hmm, pretend I used your test mods rather than my micro-mod -- definitely better to have more tests than just the one.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 723629 [details] [diff] [review]
Another try, validates as having no UB with -fsanitize=undefined, shouldn't

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

r=me with the unit test changes from my patch brought back in.

::: mfbt/CheckedInt.h
@@ -454,4 @@
>           !(IsSigned<T>::value && x == MinValue<T>::value && y == T(-1));
>  }
>  
> -// This is just to shut up msvc warnings about negating unsigned ints.

This comment should probably stay, otherwise it's too tempting for future smartasses to rip this NegateImpl helper out.

@@ +464,4 @@
>  {
> +    static CheckedInt<T> negate(const CheckedInt<T>& val)
> +    {
> +      return CheckedInt<T>(0, val.isValid() && val.mValue == 0);

So, I wasn't a fan of this OppositeIfSignedImpl->NegateImpl change making the helper bigger and a friend, but at least it has the modest advantage of yielding simpler code in the unsigned case.

By the way, I rather think that we should keep supporting unary operator- on unsigned types, because if we don't, some users writing generic code might "fix" their compile errors like this:

   result = - foo.value();

thus losing all the safety of CheckedInt.

Comment 9

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1cee7393c73

Hmm, -unsigned hackarounds.  I think if we made the error case clear enough, people wouldn't actually end up doing that.  Maybe.  More thought needed, and not necessary to fix now anyway.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e1cee7393c73