1884268 - FirstSubsumedFrame::operator=(&&) leaks principals, maybe invokes UB

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[mozillabugs]

FirstSubsumedFrame::operator=(&&) (js/public/Stack.h) leaks the object's principals member (code from FIREFOX_123_0_1_RELEASE):

    150: FirstSubsumedFrame& operator=(FirstSubsumedFrame&& rhs) {
    151:   new (this) FirstSubsumedFrame(std::move(rhs));
    152:   return *this;

    144: FirstSubsumedFrame(FirstSubsumedFrame&& rhs)
    145:     : principals(rhs.principals), ignoreSelfHosted(rhs.ignoreSelfHosted) {
    146:   MOZ_ASSERT(this != &rhs, "self move disallowed");
    147:   rhs.principals = nullptr;
    148: }

because it doesn't drop principals before creating a new object from rhs. This usage might also invoke UB because the class depends upon the destructor's side-effects. (See C++20 draft spec n4750 s.6.6.3(5).)

    155: ~FirstSubsumedFrame() {
    156:   if (principals) {
    157:     JS_DropPrincipals(cx, principals);
    158:   }
    159: }

Searchfox says that operator= (&&) isn't called.

Reported as a security bug because of the potential UB.

Comment 1

[Andrew McCreight [:mccr8]]

Code coverage does say that destructor has been run a bunch of times, for what it is worth.

Comment 2

[mozillabugs]

But it seems to indicate that operator=(&&) wasn't called? It says that it "wasn't instrumented for coverage"? What does that mean?

Comment 3

[Andrew McCreight [:mccr8]]

Oh, sorry, I totally misread what you wrote.

Comment 4

[mozillabugs]

(In reply to Andrew McCreight [:mccr8] from comment #3)

Oh, sorry, I totally misread what you wrote.

That was my fault -- it was ambiguous, so I edited it after your first response.

Comment 5

[Iain Ireland [:iain]]

n4750 says:

A program may end the lifetime of any object by reusing the storage which the object occupies or by explicitly calling the destructor for an object of a class type with a non-trivial destructor. For an object of a class type with a non-trivial destructor, the program is not required to call the destructor explicitly before the storage which the object occupies is reused or released; however, if there is no explicit call to the destructor or if a delete-expression (8.5.2.5) is not used to release the storage, the destructor shall not be implicitly called and
any program that depends on the side effects produced by the destructor has undefined behavior.

It's not immediately obvious to me what "any program that depends on the side effects" means in practice. Digging a little deeper, it seems that the C++ committee also found this wording confusing. That paragraph has been rewritten to:

A program may end the lifetime of an object of class type without invoking the destructor, by reusing or releasing the storage as described above. In this case, the destructor is not implicitly invoked. [Note: The correct behavior of a program often depends on the destructor being invoked for each object of class type. -- end note]

By my reading, simply leaking an object with a non-trivial destructor is not currently UB.

Digging into the history of this code, it looks like this bug was caught at the time and fixed by adding this->~FirstSubsumedFrame(); as the first line of operator=(&&), but then we accidentally landed an older version of the patch without the fix.

The current version seems straightforwardly broken; I think we would have caught this bug very quickly if we ever called operator=(&&).

If I'm not missing anything, I think it would be reasonable to open up this bug and land the one-line patch?

Comment 6

[mozillabugs]

By my reading, simply leaking an object with a non-trivial destructor [by reinitializing the object with placement new without previously calling the destructor] is not currently UB.

I think you're probably correct. This usage is very ugly, though, and definitely invokes UB on self-assignment:

https://eel.is/c++draft/basic.life#5:

 The lifetime of an object o of type T ends when:...(1.5) the storage which the object
occupies is released, or is reused by an object that is not nested within o ([intro.object]).

When evaluating a new-expression, storage is considered reused after it is
returned from the allocation function, but before the evaluation of the new-initializer ([expr.new]).
[Example 1: struct S {
  int m;
};

void f() {
  S x{1};
  new(&x) S(x.m);   // undefined behavior
}
— end example]

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1884268: Remove FirstSubsumedFrame::operator=(&&) r=jandem

This bug was caught and fixed when we first wrote this code, but it looks like we landed a previous version of the patch.

The alternative would be to simply delete operator= entirely, since we clearly don't use it and haven't needed it in eight years.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ec159556387
Remove FirstSubsumedFrame::operator=(&&) r=jandem

Comment 9

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/7ec159556387