Closed Bug 1168903 Opened 5 years ago Closed 5 years ago

class nsSMILValue may invoke undefined behavior in Swap (& should just switch to use Move construction)

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: q1, Assigned: dholbert)

Details

Attachments

(2 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows; rv:***) Gecko/20100101 Firefox/**.*
Build ID: 20150305021524

Steps to reproduce:

class nsSMILValue (38.0.1\dom\smil\nsSMILValue.h/.cpp) may invoke undefined behavior. nsSMILValue::Swap(nsSMILValue& aOther) swaps the contents of the objects aOther and this using memcpy:

58:  nsSMILValue tmp;
59:  memcpy(&tmp,    &aOther, sizeof(nsSMILValue));  // tmp    = aOther
60:  memcpy(&aOther, this,    sizeof(nsSMILValue));  // aOther = this
61:  memcpy(this,    &tmp,    sizeof(nsSMILValue));  // this   = tmp

This operation is valid for (only?) trivially-copyable types, per C++11 s.3.9(2&3). S.9(6) defines a trivially-copyable type as one that, among other things, has a trivial destructor. S.12.4(4) defines a trivial destructor as one that is, among other things, not user-provided.

However, nsSMILValue has a user-provided destructor, so it is not a trivally-copyable type.

The implication of s.3.9(2&3) is that it is invalid to copy a non-trivially-copyable object using memcpy. Presumably the results of doing so are undefined, and may therefore represent a security risk.
Component: Untriaged → SVG
Product: Firefox → Core
(In reply to q1 from comment #0)
> The implication of s.3.9(2&3) is that it is invalid to copy a
> non-trivially-copyable object using memcpy. Presumably the results of doing
> so are undefined, and may therefore represent a security risk.

Rather, I think the implication is that it *could* be invalid.  E.g. if the user-provided destructor deleted some memory managed by the object, then it's clearly not safe to simply copy the object using memcpy and leave the old copy around, because then you'd get a double-free, when each object is destroyed.

So, this memcpy operation *could* be invalid, if we did something like that. But we don't, so I think we're OK.
Still, it might be worthwhile to rewrite this anyway, to avoid flirting with maybe-technically-undefined behavior.

We could have written this Swap() method using the nsSMILValue copy-constructor & operator= here, but w didn't, because that would result in extra copies being generated (needlessly), for nsSMILValues that track external data.

We should really make nsSMILValue support "move" construction, and then we can use the "Swap" method defined in http://mxr.mozilla.org/mozilla-central/source/mfbt/Move.h , without any unnecessary copying of extra buffers.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm going to un-check the security-sensitive flag here, because I'm skeptical that there are any real-world vulnerabilities hiding here, per comment 1.
Summary: class nsSMILValue may invoke undefined behavior → class nsSMILValue may invoke undefined behavior in Swap (& should just switch to use Move construction)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
[blockquote]Rather, I think the implication is that it *could* be invalid.  E.g. if the user-provided destructor deleted some memory managed by the object, then it's clearly not safe to simply copy the object using memcpy and leave the old copy around, because then you'd get a double-free, when each object is destroyed.

So, this memcpy operation *could* be invalid, if we did something like that. But we don't, so I think we're OK.[/blockquote]
I think that the validity of using memcpy to copy objects depends upon the internal representation of the objects. S.3.9 guarantees that it's OK to copy trivially-copyable types with memcpy, but makes no guarantees for other types. That constrains what compiler-writers can do to implement trivially-copyable types, but leaves no constraints on how they implement other types (other than standard-layout types). Perhaps compiler X+++ adds an opaque unique random tag to each non-trivially-copyable, non-standard-layout object, which it uses to implement a lazy garbage collector. In that case, copying a tag could cause a double free.
"Perhaps", though I strongly doubt any compiler that we actually use does anything like that.

Per comment 2, though, I agree with you on a theoretical level that we should clean this up.
Agreed.
Haven't tested thoroughly yet, but I think this is correct as part 1.

(See http://mxr.mozilla.org/mozilla-central/source/mfbt/Move.h?force=1#15 for background on move constructors, if you're not familiar with them.)

Part 2 will remove nsSMILValue::Swap and replace it with Move.h's templated Swap() impl.
Attachment #8611501 - Flags: review?(bbirtles)
(updating part 1 to include bug # in commit message)
Attachment #8611501 - Attachment is obsolete: true
Attachment #8611501 - Flags: review?(bbirtles)
Attachment #8611505 - Flags: review?(bbirtles)
(Part 2 is adding an #include for Move.h to each modified file. As part of that, I'm making small tweaks to #include ordering to be in correct order -- at least, making Foo.cpp #include its own header file first, and #including mozilla/... headers at the right point alphabetically (ignoring case).
I verified in GDB that we are indeed exercising the Move constructor & assignment operator. (so we're not doing silly excess copying with the standard constructor/assignment operator)

As a bonus, I also see us exercising this new Move constructor in calls to nsTArray<nsSMILValue>::AppendElement(). (That function takes a temporary nsSMILValue and constructs a new value in the array as a copy of it, and it takes advantage of Move constructors if its passed-in argument is a temporary.)
(Thanks to q1 for noticing this & filing, BTW!)
Attachment #8611505 - Flags: review?(bbirtles) → review+
Comment on attachment 8611506 [details] [diff] [review]
part 2: Use Move.h's Swap() function instead of nsSMILValue::Swap

Looks good. Thanks for doing this.

A few of those Swaps could probably not be made a little more clear as assignment with Move() but it's a pretty marginal gain.
Attachment #8611506 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #14)
> Comment on attachment 8611506 [details] [diff] [review]
> A few of those Swaps could probably not be made a little more clear as
> assignment with Move() but it's a pretty marginal gain.

D'oh, excellent point.

I think *all* of them should be Move(), since they're generally working with a temporary value that we're done with (and don't care about its swapped-in value) after the operation.

I'll update the patch to use Move() instead of Swap() and repost for a final review. Thanks for noticing that, and sorry for the (upcoming) extra review-ping. :)
Comment on attachment 8612387 [details] [diff] [review]
part 2 v2: Use Move() instead of nsSMILValue::Swap

Canceling review, looks like I broke something in the latest Try run.
Attachment #8612387 - Flags: review?(bbirtles)
Ah, I had one accidental self-assignment: "aValue = Move(aValue)". Fixed that in this new version, & verified that the formerly-failing mochitest now passes.
Attachment #8612387 - Attachment is obsolete: true
Attachment #8612518 - Flags: review?(bbirtles)
Comment on attachment 8612518 [details] [diff] [review]
part 2 v3: Use Move() instead of nsSMILValue::Swap

Sorry about the delay (PTO today and I forgot to update my Bugzilla status).

>diff --git a/dom/smil/nsSMILAnimationFunction.cpp b/dom/smil/nsSMILAnimationFunction.cpp
>--- a/dom/smil/nsSMILAnimationFunction.cpp
>+++ b/dom/smil/nsSMILAnimationFunction.cpp
>@@ -262,17 +264,17 @@ nsSMILAnimationFunction::ComposeResult(c
>       return;
> 
>     if (NS_FAILED(AccumulateResult(values, result)))
>       return;
>   }
> 
>   // If additive animation isn't required or isn't supported, set the value.
>   if (!isAdditive || NS_FAILED(aResult.SandwichAdd(result))) {
>-    aResult.Swap(result);
>+    aResult = Move(result);
>     // Note: The old value of aResult is now in |result|, and it will get
>     // cleaned up when |result| goes out of scope, when this function returns.

This comment needs to be updated.

Apart from that, looks great! Thanks for updating this (and sorry, I just noticed my previous comment had a stray 'not' in it which probably made it quite confusing!).
Attachment #8612518 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #21)
> Sorry about the delay (PTO today and I forgot to update my Bugzilla status).

No worries! Not much of a delay :)

> This comment needs to be updated.

Good catch.

> (and sorry, I just
> noticed my previous comment had a stray 'not' in it which probably made it
> quite confusing!).

Nope, I skimmed right past it - didn't even notice until you mentioned it. :)
https://hg.mozilla.org/mozilla-central/rev/24f1455af9f2
https://hg.mozilla.org/mozilla-central/rev/b779fae94e88
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.