Commit 0a874553 authored by Ian Kilpatrick's avatar Ian Kilpatrick Committed by Commit Bot

[css-layout-api] Fix using an object which has been reallocated.

Per: crbug.com/1119873

It was possible to mutate the work queue while iterating over it. Using
this it was possible to trigger a UAF.

This patch converts CustomLayoutWorkTask to oilpan (not strictly
required as a copy of the CustomLayoutWorkTask would have also
sufficed), and now copies the Member<CustomLayoutWorkTask> before using
it.

Bug: 1119873
Change-Id: I3c66859af8c9a0f33fe8c7df7c30efd2913c2985
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2380135Reviewed-by: default avatarAlison Maher <almaher@microsoft.com>
Reviewed-by: default avatarMatthew Denton <mpdenton@chromium.org>
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809215}
parent e1eac202
......@@ -142,11 +142,14 @@ bool CSSLayoutDefinition::Instance::Layout(
}
// Run the work queue until exhaustion.
while (!custom_layout_scope->Queue()->IsEmpty()) {
for (auto& task : *custom_layout_scope->Queue()) {
task.Run(space, node.Style(), border_box_size.block_size);
auto& queue = *custom_layout_scope->Queue();
while (!queue.IsEmpty()) {
// The queue may mutate (re-allocating the vector) while running a task.
for (wtf_size_t index = 0; index < queue.size(); ++index) {
auto task = queue[index];
task->Run(space, node.Style(), border_box_size.block_size);
}
custom_layout_scope->Queue()->clear();
queue.clear();
{
v8::MicrotasksScope microtasks_scope(isolate, microtask_queue,
v8::MicrotasksScope::kRunMicrotasks);
......@@ -268,13 +271,16 @@ bool CSSLayoutDefinition::Instance::IntrinsicSizes(
}
// Run the work queue until exhaustion.
while (!custom_layout_scope->Queue()->IsEmpty()) {
for (auto& task : *custom_layout_scope->Queue()) {
task.Run(space, node.Style(),
child_percentage_resolution_block_size_for_min_max,
child_depends_on_percentage_block_size);
auto& queue = *custom_layout_scope->Queue();
while (!queue.IsEmpty()) {
// The queue may mutate (re-allocating the vector) while running a task.
for (wtf_size_t index = 0; index < queue.size(); ++index) {
auto task = queue[index];
task->Run(space, node.Style(),
child_percentage_resolution_block_size_for_min_max,
child_depends_on_percentage_block_size);
}
custom_layout_scope->Queue()->clear();
queue.clear();
{
v8::MicrotasksScope microtasks_scope(isolate, microtask_queue,
v8::MicrotasksScope::kRunMicrotasks);
......
......@@ -42,7 +42,9 @@ ScriptPromise CustomLayoutChild::intrinsicSizes(
auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
CustomLayoutScope::Current()->Queue()->emplace_back(
this, token_, resolver, CustomLayoutWorkTask::TaskType::kIntrinsicSizes);
MakeGarbageCollected<CustomLayoutWorkTask>(
this, token_, resolver,
CustomLayoutWorkTask::TaskType::kIntrinsicSizes));
return resolver->Promise();
}
......@@ -81,8 +83,9 @@ ScriptPromise CustomLayoutChild::layoutNextFragment(
auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
CustomLayoutScope::Current()->Queue()->emplace_back(
this, token_, resolver, options, std::move(constraint_data),
CustomLayoutWorkTask::TaskType::kLayoutFragment);
MakeGarbageCollected<CustomLayoutWorkTask>(
this, token_, resolver, options, std::move(constraint_data),
CustomLayoutWorkTask::TaskType::kLayoutFragment));
return resolver->Promise();
}
......
......@@ -12,7 +12,7 @@ namespace blink {
// The work queue is a list of work tasks which will either produce fragment(s)
// or intrinsic-size(s) for the custom-layout class.
typedef Vector<CustomLayoutWorkTask, 4> CustomLayoutWorkQueue;
typedef HeapVector<Member<CustomLayoutWorkTask>, 4> CustomLayoutWorkQueue;
// This heap allocated class is used to indicate which custom-layout (heap)
// objects are still valid.
......
......@@ -40,6 +40,13 @@ CustomLayoutWorkTask::CustomLayoutWorkTask(
CustomLayoutWorkTask::~CustomLayoutWorkTask() = default;
void CustomLayoutWorkTask::Trace(Visitor* visitor) const {
visitor->Trace(child_);
visitor->Trace(token_);
visitor->Trace(resolver_);
visitor->Trace(options_);
}
void CustomLayoutWorkTask::Run(
const NGConstraintSpace& parent_space,
const ComputedStyle& parent_style,
......
......@@ -8,6 +8,7 @@
#include "third_party/blink/renderer/bindings/core/v8/v8_custom_layout_constraints_options.h"
#include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
#include "third_party/blink/renderer/platform/heap/handle.h"
#include "third_party/blink/renderer/platform/heap/heap.h"
namespace blink {
......@@ -22,7 +23,8 @@ class ScriptPromiseResolver;
// Contains all the information needed to resolve a promise with a fragment or
// intrinsic-sizes.
class CustomLayoutWorkTask {
class CustomLayoutWorkTask final
: public GarbageCollected<CustomLayoutWorkTask> {
public:
enum TaskType {
kLayoutFragment,
......@@ -43,6 +45,7 @@ class CustomLayoutWorkTask {
scoped_refptr<SerializedScriptValue> constraint_data,
const TaskType type);
~CustomLayoutWorkTask();
void Trace(Visitor*) const;
// Runs this work task.
void Run(const NGConstraintSpace& parent_space,
......@@ -51,10 +54,10 @@ class CustomLayoutWorkTask {
bool* child_depends_on_percentage_block_size = nullptr);
private:
Persistent<CustomLayoutChild> child_;
Persistent<CustomLayoutToken> token_;
Persistent<ScriptPromiseResolver> resolver_;
Persistent<const CustomLayoutConstraintsOptions> options_;
Member<CustomLayoutChild> child_;
Member<CustomLayoutToken> token_;
Member<ScriptPromiseResolver> resolver_;
Member<const CustomLayoutConstraintsOptions> options_;
scoped_refptr<SerializedScriptValue> constraint_data_;
TaskType type_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment