1884457 - (CVE-2024-3862) Potential use of uninitialized storage on self-assignment in MarkStack::operator=()

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[mozillabugs]

MarkStack::operator=() (js/src/gc/Marking.cpp) uses uninitialized storage on self-assignment:

    1718: MarkStack& MarkStack::operator=(const MarkStack& other) {
    1719:   new (this) MarkStack(other);
    1720:   return *this;
    1721: }
    ...
    1734: MarkStack& MarkStack::operator=(MarkStack&& other) noexcept {
    1735:   new (this) MarkStack(std::move(other));
    1736:   return *this;
    1737: }

as the memory containing this is considered reused (and therefore of indeterminate content) upon return from the placement new function and before the new-initializer.

Searchfox says that these functions aren't called, but contradicts itself by saying that the move version has been hit over 5,000,000 times in coverage. I can't seem to find the code that calls it.

Comment 1

[Bryan Thrall [:bthrall]]

:jonco, can you evaluate this and update priority and severity correspondingly?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to mozillabugs from comment #0)

as the memory containing this is considered reused (and therefore of indeterminate content) upon return from the placement new function and before the new-initializer.

I don't see how this is using uninitialized storage. The constructor is called before placement new returns so the contents of this are not indeterminate.

Having said that, this code is not ideal and has several potential problems, although none of these occur in practice. It doesn't free the current contents of this before calling placement new (this hasn't been a problem in practice since this is either not called or only called on default initialized temporaries that don't have any allocated memory) and it doesn't handle self assignment.

The idea here was to add move construction/assignment so we could easily use std::swap to change mark stacks in GCMarker::setMarkColor. But maybe it would work out simpler to just provide a swap method.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1884457 - Replace MarkStack copy and move constructors and assignement operators with simpler swap method r?sfink

These are only used for swapping and this is simpler and more obviously correct.

Comment 4

[mozillabugs]

(In reply to Jon Coppeard (:jonco) from comment #2)

(In reply to mozillabugs from comment #0)

as the memory containing this is considered reused (and therefore of indeterminate content) upon return from the placement new function and before the new-initializer.

I don't see how this is using uninitialized storage. The constructor is called before placement new returns so the contents of this are not indeterminate.

But placement new does two things: allocate (reuse) memory, and call the constructor. Once it's done its allocation, the memory is considered reused (by which the spec also means uninitialized). From https://eel.is/c++draft/basic.life s.6.7.3(1.5) (the current draft C++ standard):

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]

The same thing would happen on self-assignment/move here.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to mozillabugs from comment #4)
Thanks for providing the spec reference.

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

I believe the problem in the example is that new-initializer x.m is not evaluated until after the allocation function has returned, which would result in reading from the now-reused memory. This does not apply to MarkStack because it doesn't read from the original allocation.

Comment 6

[mozillabugs]

(In reply to Jon Coppeard (:jonco) from comment #5)

(In reply to mozillabugs from comment #4)
Thanks for providing the spec reference.

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

I believe the problem in the example is that new-initializer x.m is not evaluated until after the allocation function has returned, which would result in reading from the now-reused memory. This does not apply to MarkStack because it doesn't read from the original allocation.

But it would on self-assignment/self-move, as in those cases other and this refer to the same location.

Comment 7

[Steve Fink [:sfink] [:s:]]

My guess:

MarkStack& MarkStack::operator=(const MarkStack& other) {
  new (this) MarkStack(other);
  return *this;
}

is not a problem when this == &other because this would call

MarkStack::MarkStack(const MarkStack& other) {
  MOZ_CRASH("Compiler requires this but doesn't call it");
}

which does not read from other (it also crashes, but I'm ignoring that.)

MarkStack& MarkStack::operator=(MarkStack&& other) noexcept {
  new (this) MarkStack(std::move(other));
  return *this;
}

on the other hand, looks like a problem if this == &other, because it does read from other:

MarkStack::MarkStack(MarkStack&& other) noexcept
    : stack_(std::move(other.stack_.ref())),
      topIndex_(other.topIndex_.ref())
#ifdef JS_GC_ZEAL
      ,
      maxCapacity_(other.maxCapacity_)
#endif
{
  other.topIndex_ = 0;
}

I don't know what the specified order of operations is here—my guess is that it evaluates std::move(other), then "blesses" the memory as an uninitialized MarkStack object, and then invokes the constructor. Which would indeed read from blessed but indeterminate memory. (Sorry, I haven't looked up spec terms for any of this.)

In practice, the memory will have its old contents, and we'll never be doing a self-assignment anyway. But it should be fixed, and Jon's fix seems good to me.

According to the hazard analysis, the first (safe, crashing) operator= has no callers. The second (move assignment) has a single caller, namely std::swap in js::GCMarker::setMarkColor(uint8) which always passes different objects. So it looks like we're good. It's a good find, though.

Comment 8

[mozillabugs]

(In reply to Steve Fink [:sfink] [:s:] from comment #7)

My guess:

MarkStack& MarkStack::operator=(const MarkStack& other) {
  new (this) MarkStack(other);
  return *this;
}

is not a problem when this == &other because this would call

MarkStack::MarkStack(const MarkStack& other) {
  MOZ_CRASH("Compiler requires this but doesn't call it");
}

Good point!

MarkStack& MarkStack::operator=(MarkStack&& other) noexcept {
  new (this) MarkStack(std::move(other));
  return *this;
}

on the other hand, looks like a problem if this == &other, because it does read from other:

MarkStack::MarkStack(MarkStack&& other) noexcept
    : stack_(std::move(other.stack_.ref())),
      topIndex_(other.topIndex_.ref())
#ifdef JS_GC_ZEAL
      ,
      maxCapacity_(other.maxCapacity_)
#endif
{
  other.topIndex_ = 0;
}

I don't know what the specified order of operations is here—my guess is that it evaluates std::move(other), then "blesses" the memory as an uninitialized MarkStack object,

I'm pretty sure it's just uninitialized memory, having no object identity at that point, as its "lifetime" (in spec-ese) has ended. As https://eel.is/c++draft/basic.life s.6.7.3(4) says: The properties ascribed to objects and references throughout this document apply for a given object or reference only during its lifetime.

and then invokes the constructor.

At which point the uninitialized memory becomes again a MarkStack object.

Which would indeed read from blessed but indeterminate memory. (Sorry, I haven't looked up spec terms for any of this.)

Yep.

Comment 9

[Jon Coppeard (:jonco)]

(In reply to mozillabugs from comment #6)

But it would on self-assignment/self-move, as in those cases other and this refer to the same location.

Ah right, yes this would be a problem. Fortunately we don't do that at the moment.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2eb8e04a0c87
Replace MarkStack copy and move constructors and assignement operators with simpler swap method r=sfink

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/2eb8e04a0c87

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR115 approval when you get a chance.

Comment 13

[Jon Coppeard (:jonco)]

Not worth uplifting IMO. The potential problematic self-assignment did not exist in the codebase.

Comment 14

[Daniel Veditz [:dveditz]]

Attachment: advisory.txt