Skip to content

Commit fb0c856

Browse files
motiz88facebook-github-bot
authored andcommitted
Fix use-after-move in console implementation
Summary: Changelog: [Internal] In the new CDP backend, calling any `console` method a second time involves a call to a moved-from `std::function`. This shouldn't work, and indeed results in an exception on some platforms, but isn't strictly an error according to the C++ standard: moving from a `std::function` leaves it in an [unspecified state](https://en.cppreference.com/w/cpp/utility/functional/function/function#:~:text=the%20call%20too.-,other,is%20in%20a%20valid%20but%20unspecified%20state%20right%20after%20the%20call.,-5), not necessarily an empty state, so (in particular) it's perfectly legal for the implementation to perform a copy instead of a move and leave the original variable intact. (See [Compiler Explorer](https://godbolt.org/z/qoo5Mnd68) for proof that libc++ and libstdc++ differ on this - the former performs a copy, while the latter actually performs a move, resulting in a `std::bad_function_call` exception later.) In the code in question, we're right to want to avoid a copy of the `body` function into the argument of `delegateExecutorSync` - only one copy of this function needs to exist at a time. But the correct way to avoid this copy is to capture `body` by reference, as we can do that repeatedly with no ill effects. (`delegateExecutorSync` is, as its name suggests, synchronous, so there are no lifetime issues.) Doing this also allows us to remove the use of `mutable` so the capturing is by *const* reference. Reviewed By: sammy-SC Differential Revision: D56673529 fbshipit-source-id: b235977b2fbc889462c4c78adfe41ae6f509e349
1 parent bb120ff commit fb0c856

File tree

2 files changed

+25
-21
lines changed

2 files changed

+25
-21
lines changed

packages/react-native/ReactCommon/jsinspector-modern/RuntimeTargetConsole.cpp

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ void RuntimeTarget::installConsoleHandler() {
155155
jsi::Runtime& runtime,
156156
const jsi::Value& thisVal,
157157
const jsi::Value* args,
158-
size_t count) mutable {
158+
size_t count) {
159159
jsi::Value retVal = innerFn(runtime, thisVal, args, count);
160160
if (originalConsole) {
161161
auto val = originalConsole->getProperty(runtime, methodName);
@@ -202,27 +202,21 @@ void RuntimeTarget::installConsoleHandler() {
202202
jsi::Runtime& runtime,
203203
const jsi::Value& /*thisVal*/,
204204
const jsi::Value* args,
205-
size_t count) mutable {
205+
size_t count) {
206206
auto timestampMs = getTimestampMs();
207-
delegateExecutorSync(
208-
[&runtime,
209-
args,
210-
count,
211-
body = std::move(body),
212-
state,
213-
timestampMs](auto& runtimeTargetDelegate) {
214-
auto stackTrace =
215-
runtimeTargetDelegate.captureStackTrace(
216-
runtime, /* framesToSkip */ 1);
217-
body(
218-
runtime,
219-
args,
220-
count,
221-
runtimeTargetDelegate,
222-
*state,
223-
timestampMs,
224-
std::move(stackTrace));
225-
});
207+
delegateExecutorSync([&](auto& runtimeTargetDelegate) {
208+
auto stackTrace =
209+
runtimeTargetDelegate.captureStackTrace(
210+
runtime, /* framesToSkip */ 1);
211+
body(
212+
runtime,
213+
args,
214+
count,
215+
runtimeTargetDelegate,
216+
*state,
217+
timestampMs,
218+
std::move(stackTrace));
219+
});
226220
return jsi::Value::undefined();
227221
})));
228222
};

packages/react-native/ReactCommon/jsinspector-modern/tests/ConsoleApiTest.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -748,6 +748,16 @@ TEST_P(ConsoleApiTest, testConsoleLogStack) {
748748
)");
749749
}
750750

751+
TEST_P(ConsoleApiTest, testConsoleLogTwice) {
752+
InSequence s;
753+
expectConsoleApiCall(
754+
AllOf(AtJsonPtr("/type", "log"), AtJsonPtr("/args/0/value", "hello")));
755+
eval("console.log('hello');");
756+
expectConsoleApiCall(AllOf(
757+
AtJsonPtr("/type", "log"), AtJsonPtr("/args/0/value", "hello again")));
758+
eval("console.log('hello again');");
759+
}
760+
751761
static const auto paramValues = testing::Values(
752762
Params{
753763
.withConsolePolyfill = true,

0 commit comments

Comments
 (0)