1439723 - Nullable::SetValue leads to footguns in the animation code

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Nathan Froyd [:froydnj]]

[Filing as a security bug because this was turned up by bug 1414901, even though I don't *think* it's exploitable.]

Over in bug 1414901, I'm making it so mozilla::Maybe<T> poisons its storage for T when there's no value being stored.  Many tests started failing--not crashing, just failing--when I tried this patch out, which seemed most peculiar.  I re-jiggered some things to make Valgrind useful and it started complaining about things like:

TEST-UNEXPECTED-FAIL | valgrind-test | Conditional jump or move depends on uninitialised value(s) at mozilla::dom::Animation::UpdateFinishedState / mozilla::dom::Animation::UpdateTiming / mozilla::dom::Animation::ResumeAt / FinishPendingAt
 ==33685== Conditional jump or move depends on uninitialised value(s)
 ==33685==    at 0xF05DCA6: mozilla::dom::Animation::UpdateFinishedState(mozilla::dom::Animation::SeekFlag, mozilla::dom::Animation::SyncNotifyFlag) (Animation.cpp:1371)
 ==33685==    by 0xF05E080: mozilla::dom::Animation::UpdateTiming(mozilla::dom::Animation::SeekFlag, mozilla::dom::Animation::SyncNotifyFlag) (Animation.cpp:1352)
 ==33685==    by 0xF05D0A4: mozilla::dom::Animation::ResumeAt(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator> const&) (Animation.cpp:1313)
 ==33685==    by 0xF05D75A: FinishPendingAt (Animation.h:413)
 ==33685==    by 0xF05D75A: mozilla::dom::Animation::Tick() (Animation.cpp:698)
...more frames follow...
 ==33685==  Uninitialised value was created by a client request
 ==33685==    at 0xF04BF1D: poison (Maybe.h:39)
 ==33685==    by 0xF04BF1D: poisonThis (Maybe.h:128)
 ==33685==    by 0xF04BF1D: mozilla::Maybe<mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator> >::reset() [clone .part.267] (Maybe.h:497)
 ==33685==    by 0xF05D70F: reset (Animation.cpp:722)
 ==33685==    by 0xF05D70F: SetValue (Nullable.h:65)
 ==33685==    by 0xF05D70F: mozilla::dom::Animation::Tick() (Animation.cpp:697)
...more frames follow...

Line 697 looks like:

    mPendingReadyTime.SetValue(std::min(mTimeline->GetCurrentTime().Value(),
                                        mPendingReadyTime.Value()));

And the particular SetValue overload getting invoked looks like:

  void SetValue(const T& aArgs)
  {
    mValue.reset();
    mValue.emplace(aArgs);
  }

What std::min does is take two references to locations, and then return a reference to the smaller value.  So what happens in some cases for the above SetValue call is that it reduces to:

  mPendingReadyTime.SetValue(mPendingReadyTime.Value());

Note that .Value() returns a reference to the value of mPendingReadyTime, not the value itself.  So mPendingReadTime.SetValue() gets a reference to the internal storage of mPendingReadyTime.  Then mPendingReadyTime.SetValue() is going to do:

1. mValue.reset(), which:
1a. Overwrites the storage of mValue with poison bits;
1b. Marks the storage of mValue as undefined for Valgrind's sake.
2. mValue.emplace(), which:
2a. reads from mPendingReadyTime's internal storage, i.e. mValue (marked as undefined/poisoned, see above)
2b. writes poison/undefined values to mPendingReadyTime's internal storage.

with the result that we get garbage in mPendingReadyTime instead of the value that we intended.  That is, we're not reading the value of mPendingReadyTime until the last possible moment, at which point the value we thought was there is not actually there.

This works OK without poisoning, because we don't really touch the internal storage of mPendingReadyTime when we reset().  (We do call the destructor for the type in the internal storage, so we're reading from destroyed memory, but we got lucky that the destructor didn't do anything to the internal contents of the object.)

The above scenario seems pretty easy to run into and a nice subtle footgun waiting for you.  bz, do you have any ideas for making Nullable::SetValue less footgun-y?

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: use a temporary in Animation::Tick()

This change avoids the use-after-destruction problem described in comment 0.
Maybe it'd be preferable to do:

  TimeDuration currentTime = mTimeline->GetCurrentTime().Value();
  if (currentTime < mPendingReadyTime.Value()) {
    mPendingReadyTime.SetValue(currentTime);
  }

instead, so we aren't doing unecessary memory writes?

Comment 2

[Boris Zbarsky [:bzbarsky]]

The only thing I've thought of so far is to remove the "const T&" overload of SetValue(), but leave the "T&&" overload.  This does require a bunch of callers to start looking like this:
 
  SomeType temp = whatever;
  foo.SetValue(Move(temp));

(as in, making the copy-construction of SomeType be more explicit).  I don't see a good way around that in general, though.  I guess we could have a SetValue(T) overload if T is some sort of primitive type or something, so we can at least avoid the annoyance when the copy is cheap.  Maybe.

Comment 3

[Matt Woodrow (:mattwoodrow)]

For the footgun, can we just compare addresses and skip the no-op operation of assigning something to itself? Or do we have an implicit contract to actually invoke the copy constructor?

If the latter, then doing Boris' idea, but internally with the T& overload would also work.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Doing that last sounds good to me.  I'd rather not add various branching in SetValue to optimize what should be a pretty rare case.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9e245003684320d69b47e44dda7b58a919a054
https://hg.mozilla.org/mozilla-central/rev/ba9e24500368