Skip to content

Commit

Permalink
Fix use-after-move in console implementation
Browse files Browse the repository at this point in the history
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.

Differential Revision: D56673529
  • Loading branch information
motiz88 authored and facebook-github-bot committed Apr 28, 2024
1 parent e2ad669 commit 22ad307
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ void RuntimeTarget::installConsoleHandler() {
jsi::Runtime& runtime,
const jsi::Value& thisVal,
const jsi::Value* args,
size_t count) mutable {
size_t count) {
jsi::Value retVal = innerFn(runtime, thisVal, args, count);
if (originalConsole) {
auto val = originalConsole->getProperty(runtime, methodName);
Expand Down Expand Up @@ -202,27 +202,21 @@ void RuntimeTarget::installConsoleHandler() {
jsi::Runtime& runtime,
const jsi::Value& /*thisVal*/,
const jsi::Value* args,
size_t count) mutable {
size_t count) {
auto timestampMs = getTimestampMs();
delegateExecutorSync(
[&runtime,
args,
count,
body = std::move(body),
state,
timestampMs](auto& runtimeTargetDelegate) {
auto stackTrace =
runtimeTargetDelegate.captureStackTrace(
runtime, /* framesToSkip */ 1);
body(
runtime,
args,
count,
runtimeTargetDelegate,
*state,
timestampMs,
std::move(stackTrace));
});
delegateExecutorSync([&](auto& runtimeTargetDelegate) {
auto stackTrace =
runtimeTargetDelegate.captureStackTrace(
runtime, /* framesToSkip */ 1);
body(
runtime,
args,
count,
runtimeTargetDelegate,
*state,
timestampMs,
std::move(stackTrace));
});
return jsi::Value::undefined();
})));
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,16 @@ TEST_P(ConsoleApiTest, testConsoleLogStack) {
)");
}

TEST_P(ConsoleApiTest, testConsoleLogTwice) {
InSequence s;
expectConsoleApiCall(
AllOf(AtJsonPtr("/type", "log"), AtJsonPtr("/args/0/value", "hello")));
eval("console.log('hello');");
expectConsoleApiCall(AllOf(
AtJsonPtr("/type", "log"), AtJsonPtr("/args/0/value", "hello again")));
eval("console.log('hello again');");
}

static const auto paramValues = testing::Values(
Params{
.withConsolePolyfill = true,
Expand Down

0 comments on commit 22ad307

Please sign in to comment.