CheckedInt unary operator- guilty of undefined behavior

RESOLVED FIXED in mozilla22



7 years ago
7 years ago


(Reporter: bjacob, Unassigned)



Firefox Tracking Flags

(Not tracked)



(2 attachments)

-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.
Attachment #723268 - Flags: review?(jwalden+bmo)
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.
Attachment #723268 - Flags: review?(jwalden+bmo) → review+
Actually, hmm, yes, this does have the warning issue still -- should have read bugmail more closely.  Thinking about that now.
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?
Wait, I fail, that doesn't avoid the warning either.  Lemme think.
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);
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.
Attachment #723629 - Flags: review?(bjacob)
Hmm, pretend I used your test mods rather than my micro-mod -- definitely better to have more tests than just the one.
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.
Attachment #723629 - Flags: review?(bjacob) → review+

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.
Target Milestone: --- → mozilla22
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.