601535 - content/media should use CheckedInt.h

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Now that CheckedInt.h exists content/media doesn't need to handroll [Add|Multiply]Overflow functions.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Nice.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Meant to take this, I have half of a patch.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This passes tests locally except test_play_twice which is permafailing on my box with and without this patch :-(

Comment 4

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

Just one question: in this line of code:

PRBool SamplesToMs(PRInt64 aSamples, PRUint32 aRate, CheckedInt64& aOutMs);

Why don't you just make aOutMs be a PRInt64& reference, and do the check internally in the body of SamplesToMs? What is the use case or rationale for exposing the integer-checking in the API?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I exposed the CheckedInt64 because most of the users of these functions go on to do other checked arithmetic with the result.  We can certainly do it that way too.

Comment 6

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

OK, that makes sense then.

Comment 7

[Matthew Gregan [:kinetik]]

It seems like CheckedInt should assert that mIsValid before returning a value from value(), otherwise it's possible for the caller to use invalid values if they forget to call valid() and check the result.  roc asked for this in bug 555798 comment 1, but it seems to have been ignored (there's no further discussion about it).

Comment 8

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

Why not. I know I considered this and somehow didn't decide to do it, but I can't remember the reason.

If you make this change, just make sure that internally in CheckedInt one always uses the mValue member and not value(), otherwise doing checked arithmetic will itself trigger your assertion.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 480553 [details] [diff] [review]
Patch

Thanks for doing this, I didn't realise we had code to do this!

* The primary gain we can get from using CheckedInt is in code clarity; since overflow errors are uncommon, we can delay checking for int overflow until after calculation is complete, and be safe in the knowledge that any errors from intermediate steps won't be forgotten.

So lets change SamplesToMs and MsToSamples to:

CheckedInt64 SamplesToMs(PRInt64 aSamples, PRUint32 aRate) {
  return (CheckedInt64(aSamples) * 1000) / aRate;
}

CheckedInt64 MsToSamples(PRInt64 aMs, PRUint32 aRate) {
  return (CheckedInt64(aMs) * aRate) / 1000;
}

We can then simplify most of the calculations to only check valid() on the resultant CheckedInt, rather than on all intermediate steps. This improves readability greatly. For example, the sampleTime/playedSamples/missingSamples calculations in nsBuiltinDecoderStateMachine::AudioLoop() can just be:

  CheckedInt64 sampleTime = MsToSamples(s->mTime, rate);
  CheckedInt64 playedSamples = MsToSamples(audioStartTime, rate) + audioDuration;
  CheckedInt64 missingSamples = sampleTime - playedSamples;
  if (!missingSamples.valid() || !sampleTime.valid()) {
    NS_WARNING("Int overflow adding in AudioLoop()");
    break;
  }

* You're using "CheckedInt<T> name(value)" some places and "CheckedInt<T> name = value;" in other places. Can we use the later? That seems clearer to me.

* Sometimes you declare:
  
  CheckedInt64 name = value;
  value += otherValue;
  
Provided it can all fit on one line, I'd argue the above would be clearer when written as:

  CheckedInt64 name = value + CheckedInt64(otherValue);
  
* In nsSkeletonState::DecodeIndex do we need to also check for overflow whenever we divide by timeDenom? (PR_INT64_MIN / -1) overflows I think.

* It's a shame we have to use .value() everywhere.

* It would be nice if CheckedInt<T> had an operator< and if its valid() function cast mIsValid to PRBool. I get lots of warnings about conversion from integer to bool while compiling with MSVC.

Comment 10

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

(In reply to comment #9)
> Comment on attachment 480553 [details] [diff] [review]
> Patch
> 
> Thanks for doing this, I didn't realise we had code to do this!

It's recent :)

> * In nsSkeletonState::DecodeIndex do we need to also check for overflow
> whenever we divide by timeDenom? (PR_INT64_MIN / -1) overflows I think.

It does, and CheckedInt will catch it.

> * It's a shame we have to use .value() everywhere.

What do you suggest instead? If we make the change proposed by comment 7, it could actually be safe to add an implicit conversion-to-plain-int operator. I just amn't 100% comfortable right now, I guess I have to add all potentially dangerous cases I can think of to the unit test (xpcom/test/TestCheckedInt.cpp).

> 
> * It would be nice if CheckedInt<T> had an operator<

I basically only implemented operators that could generate errors (overflow, etc). Feel free to add more operators, just please make sure to expand the unit test accordingly, if only to check compilation and basic sanity.

> and if its valid()
> function cast mIsValid to PRBool.

Hm,

    PRBool valid() const
    {
        return mIsValid;
    }

do you mean I should explicitly cast, like  return PRBool(mIsValid)  ?

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #10)
> > * It's a shame we have to use .value() everywhere.
> 
> What do you suggest instead? If we make the change proposed by comment 7, it
> could actually be safe to add an implicit conversion-to-plain-int operator.

Yeah, an operator<T> was what I was thinking of.

> > * It would be nice if CheckedInt<T> had an operator<
> 
> I basically only implemented operators that could generate errors (overflow,
> etc).

That's understandable. It's just a little inconvenient to have to compare checkedInt1.value() < checkedInt2.value().


> Feel free to add more operators, just please make sure to expand the unit
> test accordingly, if only to check compilation and basic sanity.

I'd feared you'd say that! ;)

> 
> > and if its valid()
> > function cast mIsValid to PRBool.
> 
> Hm,
> 
>     PRBool valid() const
>     {
>         return mIsValid;
>     }
> 
> do you mean I should explicitly cast, like  return PRBool(mIsValid)  ?

Yes. Without that, I get a 60 line warning about templates when compiling. I also get similarly large warnings whenever CheckedInt::CheckedInt<U, PRBool> is called with an int. If I change that constructor's PRBool argument to a type T I don't get any warnings.

Comment 12

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

Attachment: fix checkedint warnings

Here's a patch fixing the warning and some others (there were lots of msvc warnings).

Comment 13

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

(In reply to comment #11)
> (In reply to comment #10)
> > > * It's a shame we have to use .value() everywhere.
> > 
> > What do you suggest instead? If we make the change proposed by comment 7, it
> > could actually be safe to add an implicit conversion-to-plain-int operator.
> 
> Yeah, an operator<T> was what I was thinking of.

So, we had a chat on #developers with Kyle, and I still don't know.
 * I still can't convince myself that adding such an implicit conversion operator wouldn't open loopholes. (But I'm not the biggest c++ guru, eh).
 * That would go together with the change making it assert validity, but then, the problem is that code that seems to run fine would suddenly crash when the result is invalid. So instead of asserting validity, the really nice thing would be the have an API whereby you can't compile code that tries to get the value without checking the result, but I can't find a non-awkward such API.
 
> 
> > > * It would be nice if CheckedInt<T> had an operator<
> > 
> > I basically only implemented operators that could generate errors (overflow,
> > etc).
> 
> That's understandable. It's just a little inconvenient to have to compare
> checkedInt1.value() < checkedInt2.value().
> 
> 
> > Feel free to add more operators, just please make sure to expand the unit
> > test accordingly, if only to check compilation and basic sanity.
> 
> I'd feared you'd say that! ;)

It's easy enough though, the test is xpcom/tests/TestCheckedInt.cpp, compile with

make xpcom/tests

and run TestCheckedInt [.exe]. You'll need to make sure it finds the mozalloc library.

> 
> > 
> > > and if its valid()
> > > function cast mIsValid to PRBool.
> > 
> > Hm,
> > 
> >     PRBool valid() const
> >     {
> >         return mIsValid;
> >     }
> > 
> > do you mean I should explicitly cast, like  return PRBool(mIsValid)  ?
> 
> Yes. Without that, I get a 60 line warning about templates when compiling. I
> also get similarly large warnings whenever CheckedInt::CheckedInt<U, PRBool> is
> called with an int. If I change that constructor's PRBool argument to a type T
> I don't get any warnings.

OK, this is addressed in my above patch.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > Yeah, an operator<T> was what I was thinking of.
> 
> So, we had a chat on #developers with Kyle, and I still don't know.
>  * I still can't convince myself that adding such an implicit conversion
> operator wouldn't open loopholes. (But I'm not the biggest c++ guru, eh).

I'm also unsure, so I won't hound you about it. ;)

>  * That would go together with the change making it assert validity, but then,
> the problem is that code that seems to run fine would suddenly crash when the
> result is invalid. So instead of asserting validity, the really nice thing
> would be the have an API whereby you can't compile code that tries to get the
> value without checking the result, but I can't find a non-awkward such API.

How about instead of asserting validity (which can't be pre-determined at compile time), you assert that validity has been checked before use?

So whenever you do an arithmetic operation which could overflow, you set a validity-needs-checking-flag. Have valid() unset the validity-needs-checking-flag, and have value() assert that the validity-needs-checking-flag is not set. Basically, you're asserting that the users of CheckedInt have called valid() before they call value() after doing dangerous arithmetic.

You could make the validity-needs-checking-flag only present in debug builds, since the only assert which uses it in value() will only be present in debug builds.

Does that sound reasonable?

> > > do you mean I should explicitly cast, like  return PRBool(mIsValid)  ?
> OK, this is addressed in my above patch.

Great, thanks!

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #14)
> How about instead of asserting validity (which can't be pre-determined at
> compile time), you assert that validity has been checked before use?
> 
> So whenever you do an arithmetic operation which could overflow, you set a
> validity-needs-checking-flag. Have valid() unset the
> validity-needs-checking-flag, and have value() assert that the
> validity-needs-checking-flag is not set. Basically, you're asserting that the
> users of CheckedInt have called valid() before they call value() after doing
> dangerous arithmetic.
> 
> You could make the validity-needs-checking-flag only present in debug builds,
> since the only assert which uses it in value() will only be present in debug
> builds.
> 
> Does that sound reasonable?

This is the next thing Benoit and I discussed.  This is better in that you don't need to cause an overflow to trigger the assertion, but you still need to cause the wrong order of statements to be executed.  Ideally we'd come up with some sort of clever c++ that enforced the function ordering constraint at compile time without being excessively ugly to handle.

Comment 16

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

(In reply to comment #15)
> (In reply to comment #14)
> > How about instead of asserting validity (which can't be pre-determined at
> > compile time), you assert that validity has been checked before use?
> > 
> > So whenever you do an arithmetic operation which could overflow, you set a
> > validity-needs-checking-flag. Have valid() unset the
> > validity-needs-checking-flag, and have value() assert that the
> > validity-needs-checking-flag is not set. Basically, you're asserting that the
> > users of CheckedInt have called valid() before they call value() after doing
> > dangerous arithmetic.
> > 
> > You could make the validity-needs-checking-flag only present in debug builds,
> > since the only assert which uses it in value() will only be present in debug
> > builds.
> > 
> > Does that sound reasonable?
> 
> This is the next thing Benoit and I discussed.

Right, I was forgetting about that :)

> This is better in that you
> don't need to cause an overflow to trigger the assertion, but you still need to
> cause the wrong order of statements to be executed.  Ideally we'd come up with
> some sort of clever c++ that enforced the function ordering constraint at
> compile time without being excessively ugly to handle.

However, since I really don't see a good way of checking this at compile time without making the API cumbersome, I would r+ a patch doing as Chris describes. 

In particular:

(In reply to comment #14)
> You could make the validity-needs-checking-flag only present in debug builds,
> since the only assert which uses it in value() will only be present in debug
> builds.

I agree.

Comment 17

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

Attachment: Fix CheckedInt warnings

Updated patch: previous version had a compile error that wasn't caught by MSVC.

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 481221 [details] [diff] [review]
Fix CheckedInt warnings

a=me for checkin to mozilla-central as long as this passes all try builds

Comment 19

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

http://hg.mozilla.org/mozilla-central/rev/e149f78d5120

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this fixed?

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

No, the main patch still needs to be reworked.

Comment 22

[Steven Tseng (:Anachid)]

Attachment: Patch using CheckedInt.h

This patch oversteps the patch made by Kyle Huey (I don't know why I am not given an option to mark the previous patch as obsolete).
I made the changes suggested in comment 9 by Chris.

Comment 23

[Steven Tseng (:Anachid)]

I ran the code with the tests inside content/media and it didn't report any errors. I am not sure if how I need it to run with any additional test, if required.

Comment 24

[Steven Tseng (:Anachid)]

Attachment: Patch using CheckedInt.h

This patch oversteps the patch made by Kyle Huey (I don't know why I am not given an option to mark the previous patch as obsolete).
I made the changes suggested in comment 9 by Chris.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

bjacob: Is there a reason why we can't add to CheckedInt.h comparison operators for less than and friends to compare two CheckedInts of the same type, e.g.:

template<typename T>
inline bool operator >(const CheckedInt<T> &lhs, const CheckedInt<T>& rhs)
{
    return lhs.value() > rhs.value();
}

Then we wouldn't need to always call value() on both the LHS and RHS manually when comparing (as we have to do in Steven's patch)...

Comment 26

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

(In reply to Chris Pearce (:cpearce) from comment #25)
> bjacob: Is there a reason why we can't add to CheckedInt.h comparison
> operators for less than and friends to compare two CheckedInts of the same
> type, e.g.:
> 
> template<typename T>
> inline bool operator >(const CheckedInt<T> &lhs, const CheckedInt<T>& rhs)
> {
>     return lhs.value() > rhs.value();
> }
> 
> Then we wouldn't need to always call value() on both the LHS and RHS
> manually when comparing (as we have to do in Steven's patch)...

The problem with that is that it doesn't check that lhs and rhs are valid, so with that it becomes much easier to check that they are. Let's preserve a simple rule: for it to be safe to use a value, it must be enough to check .valid() on the same object. The above operator> breaks this simple rule. You could however add bool support to CheckedInt and make a operator> returning a CheckedInt<bool>. That would be safe.

Comment 27

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

(In reply to Benoit Jacob [:bjacob] from comment #26)
> The problem with that is that it doesn't check that lhs and rhs are valid,
> so with that it becomes much easier to check that they are.

I meant: it becomes much easier to *FORGET TO* check that they are.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed patch to TryServer, looks green:
https://tbpl.mozilla.org/?tree=Try&rev=cdbb85b9b07e

Steven, looks like you're ready for someone to land this for you?

Comment 29

[Steven Tseng (:Anachid)]

yes, please. I was wondering if you guys wanted to change some stuff with regard to the > operator.
looks like we are not gonna be touching that then.

Comment 30

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e606267898cf

Comment 31

[Ed Morley [:emorley]]

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