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

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[q1]

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 &&).

Comment 1

[q1]

> 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.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 3

[q1]

(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.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

No objection!

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Though, if anyone is doing |*this == Move(this)| I feel like we should crash... MOZ_RELEASE_ASSERT?

Comment 6

[Andrew McCreight [:mccr8]]

Somebody was adding some kind of asserts or something for self moves, but I don't recall the details.

Comment 7

[q1]

(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?

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 9

[q1]

(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?

Comment 10

[q1]

Oops, forgot the needinfo flag for the above.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Just to be clear, this bug is about operator=, right? A number of comments talk about operator==, but I think that's a mistake.

Comment 12

[q1]

(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.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

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.

Comment 15

[q1]

(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.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/877583d20734

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/877583d20734

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.