Skip to content

Commit

Permalink
Fix missing write barrier bug in ArrayStorage::pop_back
Browse files Browse the repository at this point in the history
Summary:
There was a bug in `ArrayStorage::pop_back`, where it decreases the array length
without notifying the GC.

Due to the way Hades uses a Snapshot at the Beginning write barrier, it needs to
know when an object might have become unreachable. Reducing the array length
is semantically equivalent to writing a null over an existing pointer, so it needs a write
barrier.

Other `resize`-like APIs are also vulnerable to this as well, and must be careful to use
write barriers whenever the length decreases, and fill uninitialized memory when the
length increases.

Reviewed By: neildhar

Differential Revision: D23355076

fbshipit-source-id: aa3f4292a72671ac9f8cec9bf29b522feb5bb3d4
  • Loading branch information
Riley Dulin authored and facebook-github-bot committed Aug 27, 2020
1 parent c60f962 commit b61de9d
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 6 deletions.
21 changes: 18 additions & 3 deletions include/hermes/VM/ArrayStorage.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,24 @@ class ArrayStorage final
}

/// Pop the last element off the array and return it.
HermesValue pop_back() {
assert(size() > 0 && "Can't pop from empty ArrayStorage");
return data()[size_.fetch_sub(1, std::memory_order_relaxed) - 1];
HermesValue pop_back(Runtime *runtime) {
const size_type sz = size();
assert(sz > 0 && "Can't pop from empty ArrayStorage");
HermesValue val = data()[sz - 1];
#ifdef HERMESVM_GC_HADES
// In Hades, a snapshot write barrier must be executed on the value that is
// conceptually being changed to null. The write doesn't need to occur, but
// it is the only correct way to use the write barrier.
data()[sz - 1].set(HermesValue::encodeEmptyValue(), &runtime->getHeap());
#else
(void)runtime;
#endif
// The background thread can't mutate size, so we don't need fetch_sub here.
// Relaxed is fine, because the GC doesn't care about the order of seeing
// the length and the individual elements, as long as illegal HermesValues
// aren't written there (which they won't be).
size_.store(sz - 1, std::memory_order_relaxed);
return val;
}

/// Ensure that the capacity of the array is at least \p capacity,
Expand Down
4 changes: 2 additions & 2 deletions lib/VM/JSLib/RuntimeJSONUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1049,7 +1049,7 @@ ExecutionStatus JSONStringifyer::operationJO() {
marker.flush();
auto result = operationStr(*tmpHandle_);

operationJOK_ = vmcast<JSArray>(stackJO_->pop_back());
operationJOK_ = vmcast<JSArray>(stackJO_->pop_back(runtime_));

if (LLVM_UNLIKELY(result == ExecutionStatus::EXCEPTION)) {
return ExecutionStatus::EXCEPTION;
Expand Down Expand Up @@ -1104,7 +1104,7 @@ CallResult<bool> JSONStringifyer::pushValueToStack(HermesValue value) {

void JSONStringifyer::popValueFromStack() {
assert(stackValue_->size() && "Cannot pop from an empty stack");
stackValue_->pop_back();
stackValue_->pop_back(runtime_);
}

void JSONStringifyer::appendToOutput(SymbolID identifierID) {
Expand Down
2 changes: 1 addition & 1 deletion lib/VM/Runtime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1313,7 +1313,7 @@ CallResult<bool> Runtime::insertVisitedObject(Handle<JSObject> obj) {
void Runtime::removeVisitedObject(Handle<JSObject> obj) {
(void)obj;
auto stack = Handle<ArrayStorage>::vmcast(&stringCycleCheckVisited_);
auto elem = stack->pop_back();
auto elem = stack->pop_back(this);
(void)elem;
assert(
elem.isObject() && elem.getObject() == obj.get() &&
Expand Down

0 comments on commit b61de9d

Please sign in to comment.