Skip to content

Commit

Permalink
Prevent scoped probe parameters from binding to rvalues
Browse files Browse the repository at this point in the history
Scoped probes are stack-only, so generally retaining references
in scoped probes is ok. However, some scoped probes were retaining
references to rvalues produced while evaluating constructor arguments,
which results in a stack UaF in case such value is referenced outside
of `Will(Probe&)` methods.

This accepts references passed to probes via std::reference_wrapper<>
that does not bind to rvalues and fixes few issues that were detected.

Bug: 1475637
Change-Id: I667a761ede9ad75bb3384148a9365b4f088b5d6e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4811809
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Auto-Submit: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1189098}
  • Loading branch information
caseq authored and Chromium LUCI CQ committed Aug 28, 2023
1 parent 1b04998 commit f2f3eb2
Show file tree
Hide file tree
Showing 9 changed files with 122 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,8 @@ v8::MaybeLocal<v8::Value> V8ScriptRunner::RunCompiledScript(

// ToCoreString here should be zero copy due to externalized string
// unpacked.
probe::ExecuteScript probe(context, isolate->GetCurrentContext(),
ToCoreString(script_url),
String url = ToCoreString(script_url);
probe::ExecuteScript probe(context, isolate->GetCurrentContext(), url,
script->GetUnboundScript()->GetId());
result = script->Run(isolate->GetCurrentContext(), host_defined_options);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ def __init__(self, source):
self.type = param_decl
self.name = build_param_name(self.type)

self.is_reference = "&" in self.type
if self.type[-1] == "*" and "char" not in self.type:
self.member_type = "Member<%s>" % self.type[:-1]
else:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,12 @@ void {{sink_class}}::Trace(Visitor* visitor) const
namespace probe {
{% macro params_list(probe) -%}
{%- for param in probe.params %}
{{param.type}} {{param.name}}
{%- if probe.is_scoped and param.is_reference %}
std::reference_wrapper<std::remove_reference_t<{{param.type}}>>
{%- else %}
{{param.type}}
{%- endif %}
{{param.name}}
{%- if not loop.last %}, {% endif -%}
{%- endfor -%}
{%- endmacro %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ namespace probe {

{%- macro params_decl(probe) %}
{%- for param in probe.params %}
{%- if probe.is_scoped and param.is_reference %}
std::reference_wrapper<std::remove_reference_t<{{param.type}}>>
{%- else %}
{{ param.type }}
{%- endif %}
{%- if param.default_value %} = {{ param.default_value }}
{%- endif %}
{%- if not loop.last %}, {% endif %}
Expand Down
78 changes: 57 additions & 21 deletions third_party/blink/renderer/core/probe/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,45 @@
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

import("//build/nocompile.gni")
import("//third_party/blink/renderer/core/core.gni")

action("instrumentation_probes") {
script = "../../build/scripts/make_instrumenting_probes.py"
template("probe_generator") {
assert(defined(invoker.probe_set))
assert(defined(invoker.probe_sink))
assert(defined(invoker.output))

inputs = [
"core_probes.pidl",
"core_probes.json5",
"../../build/scripts/templates/instrumenting_probes_impl.cc.tmpl",
"../../build/scripts/templates/instrumenting_probes_inl.h.tmpl",
"../../build/scripts/templates/probe_sink.h.tmpl",
]
action(target_name) {
script = "../../build/scripts/make_instrumenting_probes.py"

outputs = [
"$blink_core_output_dir/core_probes_inl.h",
"$blink_core_output_dir/core_probes_impl.cc",
"$blink_core_output_dir/core_probe_sink.h",
]
inputs = [
"${invoker.probe_set}.pidl",
"${invoker.probe_set}.json5",
"../../build/scripts/templates/instrumenting_probes_impl.cc.tmpl",
"../../build/scripts/templates/instrumenting_probes_inl.h.tmpl",
"../../build/scripts/templates/probe_sink.h.tmpl",
]

args = [
rebase_path(inputs[0], root_build_dir),
"--config",
rebase_path("core_probes.json5", root_build_dir),
"--output_dir",
rebase_path(blink_core_output_dir, root_build_dir),
]
outputs = [
"${invoker.output}/${invoker.probe_set}_inl.h",
"${invoker.output}/${invoker.probe_set}_impl.cc",
"${invoker.output}/${invoker.probe_sink}.h",
]

args = [
rebase_path(inputs[0], root_build_dir),
"--config",
rebase_path("${invoker.probe_set}.json5", root_build_dir),
"--output_dir",
rebase_path(invoker.output, root_build_dir),
]
}
}

probe_generator("instrumentation_probes") {
probe_set = "core_probes"
probe_sink = "core_probe_sink"
output = blink_core_output_dir
}

source_set("generated") {
Expand All @@ -54,3 +67,26 @@ blink_core_sources("probe") {
"//v8",
]
}

if (enable_nocompile_tests) {
probe_generator("test_probes") {
probe_set = "test_probes"
probe_sink = "test_probe_sink"
output = "${blink_core_output_dir}/probe"
}

nocompile_test("blink_probes_nocompile_tests") {
sources = [ "probes_test.nc" ]

deps = [
":test_probes",
"//base",
"//base/test:run_all_unittests",
"//testing/gtest",
]
include_dirs = [
root_gen_dir,
"//v8/include",
]
}
}
6 changes: 3 additions & 3 deletions third_party/blink/renderer/core/probe/core_probes.pidl
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,11 @@ interface CoreProbes {
void WillHandlePromise([Keep] ExecutionContext* context, ScriptState* script_state, bool resolving, const char* class_like, const char* promise_like);
RecalculateStyle(Document* document);
UpdateLayout(Document* document);
ExecuteScript([Keep] ExecutionContext* context, const v8::Local<v8::Context>& v8_context, const String& script_url, int script_id);
ExecuteScript([Keep] ExecutionContext* context, v8::Local<v8::Context> v8_context, const String& script_url, int script_id);
CompileAndRunScript([Keep] ExecutionContext* context, const blink::Script* script);

CallFunction([Keep] ExecutionContext* context, const v8::Local<v8::Context>& v8_context, v8::Local<v8::Function> function, int depth = 0);
UserCallback([Keep] ExecutionContext* context, const char* name, const AtomicString& atomic_name, bool recurring, EventTarget* event_target = nullptr, Event* event = nullptr, EventListener* listener = nullptr);
CallFunction([Keep] ExecutionContext* context, v8::Local<v8::Context> v8_context, v8::Local<v8::Function> function, int depth = 0);
UserCallback([Keep] ExecutionContext* context, const char* name, AtomicString atomic_name, bool recurring, EventTarget* event_target = nullptr, Event* event = nullptr, EventListener* listener = nullptr);
InvokeCallback([Keep] ExecutionContext* context, const char* name, CallbackFunctionBase* callback, v8::MaybeLocal<v8::Value> function = v8::MaybeLocal<v8::Value>());
InvokeEventHandler([Keep] ExecutionContext* context, EventTarget* event_target = nullptr, Event* event = nullptr, EventListener* listener = nullptr);
V8Compile([Keep] ExecutionContext* context, const String& file_name, int line, int column);
Expand Down
26 changes: 26 additions & 0 deletions third_party/blink/renderer/core/probe/probes_test.nc
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// This is a "No Compile Test" suite.
// http://dev.chromium.org/developers/testing/no-compile-tests

#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"

struct ProbeBase { };
class TestProbeSink;

// Generated include should appear after all dependencies.
#include "third_party/blink/renderer/core/probe/test_probes_inl.h"

namespace blink {
namespace probe {

#if defined(NCTEST_SCOPED_PROBE_CONSTRUCTOR_CALLED_WITH_RVALUE) // [r"fatal error: no matching constructor for initialization of 'probe::Frobnicate'"]
void WontCompile() {
probe::Frobnicate scoped_probe((String()));
}
#endif

}
}
16 changes: 16 additions & 0 deletions third_party/blink/renderer/core/probe/test_probes.json5
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
settings: {
export_header: "third_party/blink/renderer/core/core_export.h",
export_symbol: "",
include_path: "third_party/blink/renderer/core/inspector",
includes: [
],
},
observers: {
DummyObserver: {
probes: [
"Frobnicate",
]
},
}
}
7 changes: 7 additions & 0 deletions third_party/blink/renderer/core/probe/test_probes.pidl
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

interface TestProbes {
Frobnicate(const String& arg);
}

0 comments on commit f2f3eb2

Please sign in to comment.