Closed Bug 1176096 Opened 6 years ago Closed 6 years ago

InterruptFrame& operator=(InterruptFrame&& aOther) creates wild pointer after destroy on self-move

Categories

(Core :: IPC, defect)

38 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: q1, Assigned: billm)

Details

Attachments

(1 file)

On self-move, operator==(InterruptFrame&& aOther) (38.0.1\ipc\glue\MessageChannel.cpp) destroys this, then creates a new object in the same memory. This has the effect of freeing the object pointed to by this->mMessageName. That object is used by another member function:

162:    InterruptFrame& operator=(InterruptFrame&& aOther)
163:    {
164:        MOZ_ASSERT(&aOther != this);
165:        this->~InterruptFrame();
166:        new (this) InterruptFrame(mozilla::Move(aOther));
167:        return *this;
168:    }

154:    ~InterruptFrame()
155:    {
156:        MOZ_ASSERT_IF(!mMessageName, mMoved);
157:
158:        if (mMessageName)
159:            free(const_cast<char*>(mMessageName));
160:    }

The assert on line 164 should be "if (&aOther == this) { return *this; }".

This appears to be a latent bug, because generated code for operator==(InterruptFrame&& aOther) does not seem to be present in the 38.0.1 executable. I notice, however, that class MessageChannel (38.0.1\ipc\glue\MessageChannel.h) has a member of type Mozilla::Vector<InterruptFrame>:

672:        mozilla::Vector<InterruptFrame> mCxxStackFrames;

If mozilla::Vector is like std::vector, certain operations on it would require that InterruptFrame be MoveAssignable, and therefore would call operator=(InterruptFrame &&).
> This appears to be a latent bug, because generated code for
> operator==(InterruptFrame&& aOther) does not seem to be present in the
> 38.0.1 executable.

Or the function could be present and used, but as inlined into a caller.
I agree that "if (&aOther == this) { return *this; }" is probably a nicer. However, since we have no evidence that the assertion is ever violated, it's not really a bug per se.

Ben, any objection to changing this code?
Flags: needinfo?(bent.mozilla)
(In reply to Bill McCloskey (:billm) from comment #2)
> I agree that "if (&aOther == this) { return *this; }" is probably a nicer.
> However, since we have no evidence that the assertion is ever violated, it's
> not really a bug per se.

I think the assertion is not present in release builds.
No objection!
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(wmccloskey)
Though, if anyone is doing |*this == Move(this)| I feel like we should crash... MOZ_RELEASE_ASSERT?
Summary: InterruptFrame& operator==(InterruptFrame&& aOther) creates wild pointer after destroy on self-move → InterruptFrame& operator=(InterruptFrame&& aOther) creates wild pointer after destroy on self-move
Somebody was adding some kind of asserts or something for self moves, but I don't recall the details.
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #5)
> Though, if anyone is doing |*this == Move(this)| I feel like we should
> crash... MOZ_RELEASE_ASSERT?

Couldn't this occur if the object gets put into standard containers and used in certain ways? Used as a target of std::sort?
Flags: needinfo?(bent.mozilla)
I don't know... If so you would expect to hit the assertion in debug builds too. In any case I'd rather know (via crash reports) than guess.
Flags: needinfo?(bent.mozilla)
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #8)
> I don't know... If so you would expect to hit the assertion in debug builds
> too. In any case I'd rather know (via crash reports) than guess.

Since self-move is harmless if implemented as a no-op, isn't the only possible impact a very slight reduction in performance?
Oops, forgot the needinfo flag for the above.
Flags: needinfo?(bent.mozilla)
Just to be clear, this bug is about operator=, right? A number of comments talk about operator==, but I think that's a mistake.
(In reply to Bill McCloskey (:billm) from comment #11)
> Just to be clear, this bug is about operator=, right? A number of comments
> talk about operator==, but I think that's a mistake.

Yes, operator=. The "==" was my flub.
(In reply to q1 from comment #9)
> Since self-move is harmless if implemented as a no-op, isn't the only
> possible impact a very slight reduction in performance?

Yeah, I think so.

Can we unhide this bug? I don't think we have any evidence that this is happening today.
Flags: needinfo?(bent.mozilla)
Group: core-security
Attached patch patchSplinter Review
As best as I can tell, the standard library should not perform this sort of self-move assignment. GCC bug 49559 specifically fixes a bug where sorting was causing a self-move-assignment. This reddit thread suggests that MSVC fixed a similar bug:
https://www.reddit.com/r/cpp/comments/357i3q/any_bestpractice_guidelines_on_moveassignment/

So it seems like the standard library isn't supposed to do this to us. I don't see any explicit statement saying so in the spec though.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Flags: needinfo?(wmccloskey)
Attachment #8627365 - Flags: review?(bent.mozilla)
Attachment #8627365 - Flags: review?(bent.mozilla) → review+
(In reply to Bill McCloskey (:billm) from comment #14)
> So it seems like the standard library isn't supposed to do this to us. I
> don't see any explicit statement saying so in the spec though.

I see something, but not exactly that. Following the GCC bug report you provided, I found C++11 s.17.6.4.9, which says, in pertinent part:

---
Each of the following applies to all arguments to functions defined in the C ++ standard library, unless explicitly stated otherwise:...

__If a function argument binds to an rvalue reference parameter, the implementation may assume that
this parameter is a unique reference to this argument. [Note: If the parameter is a generic parameter of the form T&& and an lvalue of type A is bound, the argument binds to an lvalue reference (14.8.2.1) and thus is not covered by the previous sentence. —end note ]
---

The "Note:" exception means that misbehaving on self-move can sometimes cause problems. Also, 17.6.4.9 covers only the STL, not other code that Mozilla might import.

I'm firmly on the defensive coding side here: make it a no-op so that Mozilla code works properly even if other code does something unexpected (but permissible!). Also doing so is easy, low risk, and has a vanishingly small impact on performance.
Flags: needinfo?(wmccloskey)
https://hg.mozilla.org/mozilla-central/rev/877583d20734
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
There's really very little harm in using an assertion here instead of a check. If there's any code out there that triggers the assertion, we'll find out in crash-stats. I'd rather know than not know.
Flags: needinfo?(wmccloskey)
You need to log in before you can comment on or make changes to this bug.