Skip to content

Commit f5de0c6

Browse files
AztecBotAztecBot
authored
feat: merge-train/avm (#18814)
BEGIN_COMMIT_OVERRIDE fix(avm): napi callbacks to ts contracts db should use shared pointers to avoid crashes on timeouts/errors (#18785) END_COMMIT_OVERRIDE
2 parents 12e4868 + 6bf64a9 commit f5de0c6

File tree

3 files changed

+103
-89
lines changed

3 files changed

+103
-89
lines changed

‎barretenberg/cpp/src/barretenberg/nodejs_module/avm_simulate/ts_callback_contract_db.cpp‎

Lines changed: 51 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -156,24 +156,23 @@ void TsCallbackContractDB::create_checkpoint()
156156

157157
try {
158158
// Call the TypeScript callback with no arguments
159-
auto result =
160-
invoke_ts_callback_with_promise(create_checkpoint_callback_,
161-
"create_checkpoint",
162-
[](Napi::Env env, Napi::Function js_callback, CallbackResults* data) {
163-
auto js_result = js_callback.Call({});
164-
165-
if (!js_result.IsPromise()) {
166-
data->error_message =
167-
"TypeScript callback did not return a Promise";
168-
data->result_promise.set_value(std::nullopt);
169-
return;
170-
}
171-
172-
auto promise = js_result.As<Napi::Promise>();
173-
auto resolve_handler = create_void_resolve_handler(env, data);
174-
auto reject_handler = create_reject_handler(env, data);
175-
attach_promise_handlers(promise, resolve_handler, reject_handler, data);
176-
});
159+
auto result = invoke_ts_callback_with_promise(
160+
create_checkpoint_callback_,
161+
"create_checkpoint",
162+
[](Napi::Env env, Napi::Function js_callback, std::shared_ptr<CallbackResults> data) {
163+
auto js_result = js_callback.Call({});
164+
165+
if (!js_result.IsPromise()) {
166+
data->error_message = "TypeScript callback did not return a Promise";
167+
data->result_promise.set_value(std::nullopt);
168+
return;
169+
}
170+
171+
auto promise = js_result.As<Napi::Promise>();
172+
auto resolve_handler = create_void_resolve_handler(env, data);
173+
auto reject_handler = create_reject_handler(env, data);
174+
attach_promise_handlers(promise, resolve_handler, reject_handler);
175+
});
177176
} catch (const std::exception& e) {
178177
throw std::runtime_error(std::string("Failed to create checkpoint: ") + e.what());
179178
}
@@ -189,24 +188,23 @@ void TsCallbackContractDB::commit_checkpoint()
189188

190189
try {
191190
// Call the TypeScript callback with no arguments
192-
auto result =
193-
invoke_ts_callback_with_promise(commit_checkpoint_callback_,
194-
"commit_checkpoint",
195-
[](Napi::Env env, Napi::Function js_callback, CallbackResults* data) {
196-
auto js_result = js_callback.Call({});
197-
198-
if (!js_result.IsPromise()) {
199-
data->error_message =
200-
"TypeScript callback did not return a Promise";
201-
data->result_promise.set_value(std::nullopt);
202-
return;
203-
}
204-
205-
auto promise = js_result.As<Napi::Promise>();
206-
auto resolve_handler = create_void_resolve_handler(env, data);
207-
auto reject_handler = create_reject_handler(env, data);
208-
attach_promise_handlers(promise, resolve_handler, reject_handler, data);
209-
});
191+
auto result = invoke_ts_callback_with_promise(
192+
commit_checkpoint_callback_,
193+
"commit_checkpoint",
194+
[](Napi::Env env, Napi::Function js_callback, std::shared_ptr<CallbackResults> data) {
195+
auto js_result = js_callback.Call({});
196+
197+
if (!js_result.IsPromise()) {
198+
data->error_message = "TypeScript callback did not return a Promise";
199+
data->result_promise.set_value(std::nullopt);
200+
return;
201+
}
202+
203+
auto promise = js_result.As<Napi::Promise>();
204+
auto resolve_handler = create_void_resolve_handler(env, data);
205+
auto reject_handler = create_reject_handler(env, data);
206+
attach_promise_handlers(promise, resolve_handler, reject_handler);
207+
});
210208
} catch (const std::exception& e) {
211209
throw std::runtime_error(std::string("Failed to commit checkpoint: ") + e.what());
212210
}
@@ -222,24 +220,23 @@ void TsCallbackContractDB::revert_checkpoint()
222220

223221
try {
224222
// Call the TypeScript callback with no arguments
225-
auto result =
226-
invoke_ts_callback_with_promise(revert_checkpoint_callback_,
227-
"revert_checkpoint",
228-
[](Napi::Env env, Napi::Function js_callback, CallbackResults* data) {
229-
auto js_result = js_callback.Call({});
230-
231-
if (!js_result.IsPromise()) {
232-
data->error_message =
233-
"TypeScript callback did not return a Promise";
234-
data->result_promise.set_value(std::nullopt);
235-
return;
236-
}
237-
238-
auto promise = js_result.As<Napi::Promise>();
239-
auto resolve_handler = create_void_resolve_handler(env, data);
240-
auto reject_handler = create_reject_handler(env, data);
241-
attach_promise_handlers(promise, resolve_handler, reject_handler, data);
242-
});
223+
auto result = invoke_ts_callback_with_promise(
224+
revert_checkpoint_callback_,
225+
"revert_checkpoint",
226+
[](Napi::Env env, Napi::Function js_callback, std::shared_ptr<CallbackResults> data) {
227+
auto js_result = js_callback.Call({});
228+
229+
if (!js_result.IsPromise()) {
230+
data->error_message = "TypeScript callback did not return a Promise";
231+
data->result_promise.set_value(std::nullopt);
232+
return;
233+
}
234+
235+
auto promise = js_result.As<Napi::Promise>();
236+
auto resolve_handler = create_void_resolve_handler(env, data);
237+
auto reject_handler = create_reject_handler(env, data);
238+
attach_promise_handlers(promise, resolve_handler, reject_handler);
239+
});
243240
} catch (const std::exception& e) {
244241
throw std::runtime_error(std::string("Failed to revert checkpoint: ") + e.what());
245242
}

‎barretenberg/cpp/src/barretenberg/nodejs_module/avm_simulate/ts_callback_utils.cpp‎

Lines changed: 38 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,10 @@ std::string extract_error_from_napi_value(const Napi::CallbackInfo& cb_info)
2525
return "Unknown error from TypeScript";
2626
}
2727

28-
Napi::Function create_buffer_resolve_handler(Napi::Env env, CallbackResults* cb_results)
28+
Napi::Function create_buffer_resolve_handler(Napi::Env env, std::shared_ptr<CallbackResults> cb_results)
2929
{
30+
// Capture shared_ptr by value to ensure CallbackResults outlives the Promise handler.
31+
// This prevents use-after-free when timeouts occur before the Promise resolves.
3032
return Napi::Function::New(
3133
env,
3234
[cb_results](const Napi::CallbackInfo& cb_info) -> Napi::Value {
@@ -56,8 +58,9 @@ Napi::Function create_buffer_resolve_handler(Napi::Env env, CallbackResults* cb_
5658
"resolveHandler");
5759
}
5860

59-
Napi::Function create_string_resolve_handler(Napi::Env env, CallbackResults* cb_results)
61+
Napi::Function create_string_resolve_handler(Napi::Env env, std::shared_ptr<CallbackResults> cb_results)
6062
{
63+
// Capture shared_ptr by value to ensure CallbackResults outlives the Promise handler.
6164
return Napi::Function::New(
6265
env,
6366
[cb_results](const Napi::CallbackInfo& cb_info) -> Napi::Value {
@@ -87,8 +90,9 @@ Napi::Function create_string_resolve_handler(Napi::Env env, CallbackResults* cb_
8790
"resolveHandler");
8891
}
8992

90-
Napi::Function create_void_resolve_handler(Napi::Env env, CallbackResults* cb_results)
93+
Napi::Function create_void_resolve_handler(Napi::Env env, std::shared_ptr<CallbackResults> cb_results)
9194
{
95+
// Capture shared_ptr by value to ensure CallbackResults outlives the Promise handler.
9296
return Napi::Function::New(
9397
env,
9498
[cb_results](const Napi::CallbackInfo& cb_info) -> Napi::Value {
@@ -98,8 +102,9 @@ Napi::Function create_void_resolve_handler(Napi::Env env, CallbackResults* cb_re
98102
"resolveHandler");
99103
}
100104

101-
Napi::Function create_reject_handler(Napi::Env env, CallbackResults* cb_results)
105+
Napi::Function create_reject_handler(Napi::Env env, std::shared_ptr<CallbackResults> cb_results)
102106
{
107+
// Capture shared_ptr by value to ensure CallbackResults outlives the Promise handler.
103108
return Napi::Function::New(
104109
env,
105110
[cb_results](const Napi::CallbackInfo& cb_info) -> Napi::Value {
@@ -110,16 +115,11 @@ Napi::Function create_reject_handler(Napi::Env env, CallbackResults* cb_results)
110115
"rejectHandler");
111116
}
112117

113-
void attach_promise_handlers(Napi::Promise promise,
114-
Napi::Function resolve_handler,
115-
Napi::Function reject_handler,
116-
CallbackResults* cb_results)
118+
void attach_promise_handlers(Napi::Promise promise, Napi::Function resolve_handler, Napi::Function reject_handler)
117119
{
118120
auto then_prop = promise.Get("then");
119121
if (!then_prop.IsFunction()) {
120-
cb_results->error_message = "Promise does not have .then() method";
121-
cb_results->result_promise.set_value(std::nullopt);
122-
return;
122+
throw std::runtime_error("Promise does not have .then() method");
123123
}
124124

125125
auto then_fn = then_prop.As<Napi::Function>();
@@ -149,32 +149,37 @@ template <typename T> T deserialize_from_msgpack(const std::vector<uint8_t>& dat
149149
std::optional<std::vector<uint8_t>> invoke_ts_callback_with_promise(
150150
const Napi::ThreadSafeFunction& callback,
151151
const std::string& operation_name,
152-
std::function<void(Napi::Env, Napi::Function, CallbackResults*)> call_js_function,
152+
std::function<void(Napi::Env, Napi::Function, std::shared_ptr<CallbackResults>)> call_js_function,
153153
std::chrono::seconds timeout)
154154
{
155-
// Create promise/future pair for synchronization
155+
// Create promise/future pair for synchronization.
156+
// The shared_ptr is passed to call_js_function which MUST capture it in Promise handlers.
157+
// This ensures CallbackResults outlives the Promise, even if we timeout and return early.
156158
auto callback_data = std::make_shared<CallbackResults>();
157159
auto future = callback_data->result_promise.get_future();
158160

159-
// Call TypeScript callback on the JS main thread
161+
// Call TypeScript callback on the JS main thread.
162+
// We pass the shared_ptr to the call_js_function so it can be captured by Promise handlers.
160163
auto status = callback.BlockingCall(
161164
callback_data.get(),
162-
[call_js_function](Napi::Env env, Napi::Function js_callback, CallbackResults* cb_results) {
165+
[call_js_function, callback_data](Napi::Env env, Napi::Function js_callback, CallbackResults* /*cb_results*/) {
163166
try {
164-
// Call the TypeScript function with appropriate arguments
165-
call_js_function(env, js_callback, cb_results);
167+
// Call the TypeScript function with the shared_ptr (not raw pointer).
168+
// The call_js_function MUST capture this shared_ptr in Promise handlers.
169+
call_js_function(env, js_callback, callback_data);
166170

167171
} catch (const std::exception& e) {
168-
cb_results->error_message = std::string("Exception calling TypeScript: ") + e.what();
169-
cb_results->result_promise.set_value(std::nullopt);
172+
callback_data->error_message = std::string("Exception calling TypeScript: ") + e.what();
173+
callback_data->result_promise.set_value(std::nullopt);
170174
}
171175
});
172176

173177
if (status != napi_ok) {
174178
throw std::runtime_error("Failed to invoke TypeScript callback for " + operation_name);
175179
}
176180

177-
// Wait for the promise to be fulfilled (with timeout)
181+
// Wait for the promise to be fulfilled (with timeout).
182+
// If timeout occurs, we throw but callback_data stays alive via shared_ptr in Promise handlers.
178183
auto wait_status = future.wait_for(timeout);
179184
if (wait_status == std::future_status::timeout) {
180185
throw std::runtime_error("Timeout waiting for TypeScript callback for " + operation_name);
@@ -196,7 +201,9 @@ std::optional<std::vector<uint8_t>> invoke_single_string_callback(const Napi::Th
196201
const std::string& operation_name)
197202
{
198203
return invoke_ts_callback_with_promise(
199-
callback, operation_name, [input_str](Napi::Env env, Napi::Function js_callback, CallbackResults* cb_results) {
204+
callback,
205+
operation_name,
206+
[input_str](Napi::Env env, Napi::Function js_callback, std::shared_ptr<CallbackResults> cb_results) {
200207
auto js_input = Napi::String::New(env, input_str);
201208
auto js_result = js_callback.Call({ js_input });
202209

@@ -207,9 +214,10 @@ std::optional<std::vector<uint8_t>> invoke_single_string_callback(const Napi::Th
207214
}
208215

209216
auto promise = js_result.As<Napi::Promise>();
217+
// Pass shared_ptr to handlers so CallbackResults outlives the Promise
210218
auto resolve_handler = create_buffer_resolve_handler(env, cb_results);
211219
auto reject_handler = create_reject_handler(env, cb_results);
212-
attach_promise_handlers(promise, resolve_handler, reject_handler, cb_results);
220+
attach_promise_handlers(promise, resolve_handler, reject_handler);
213221
});
214222
}
215223

@@ -221,7 +229,8 @@ std::optional<std::vector<uint8_t>> invoke_double_string_callback(const Napi::Th
221229
return invoke_ts_callback_with_promise(
222230
callback,
223231
operation_name,
224-
[input_str1, input_str2](Napi::Env env, Napi::Function js_callback, CallbackResults* cb_results) {
232+
[input_str1,
233+
input_str2](Napi::Env env, Napi::Function js_callback, std::shared_ptr<CallbackResults> cb_results) {
225234
auto js_input1 = Napi::String::New(env, input_str1);
226235
auto js_input2 = Napi::String::New(env, input_str2);
227236
auto js_result = js_callback.Call({ js_input1, js_input2 });
@@ -233,9 +242,10 @@ std::optional<std::vector<uint8_t>> invoke_double_string_callback(const Napi::Th
233242
}
234243

235244
auto promise = js_result.As<Napi::Promise>();
245+
// Pass shared_ptr to handlers so CallbackResults outlives the Promise
236246
auto resolve_handler = create_string_resolve_handler(env, cb_results);
237247
auto reject_handler = create_reject_handler(env, cb_results);
238-
attach_promise_handlers(promise, resolve_handler, reject_handler, cb_results);
248+
attach_promise_handlers(promise, resolve_handler, reject_handler);
239249
});
240250
}
241251

@@ -246,7 +256,8 @@ void invoke_buffer_void_callback(const Napi::ThreadSafeFunction& callback,
246256
auto result = invoke_ts_callback_with_promise(
247257
callback,
248258
operation_name,
249-
[buffer_data = std::move(buffer_data)](Napi::Env env, Napi::Function js_callback, CallbackResults* cb_results) {
259+
[buffer_data = std::move(buffer_data)](
260+
Napi::Env env, Napi::Function js_callback, std::shared_ptr<CallbackResults> cb_results) {
250261
auto js_buffer = Napi::Buffer<uint8_t>::Copy(env, buffer_data.data(), buffer_data.size());
251262
auto js_result = js_callback.Call({ js_buffer });
252263

@@ -257,9 +268,10 @@ void invoke_buffer_void_callback(const Napi::ThreadSafeFunction& callback,
257268
}
258269

259270
auto promise = js_result.As<Napi::Promise>();
271+
// Pass shared_ptr to handlers so CallbackResults outlives the Promise
260272
auto resolve_handler = create_void_resolve_handler(env, cb_results);
261273
auto reject_handler = create_reject_handler(env, cb_results);
262-
attach_promise_handlers(promise, resolve_handler, reject_handler, cb_results);
274+
attach_promise_handlers(promise, resolve_handler, reject_handler);
263275
});
264276

265277
// For void callbacks, we just need to ensure no errors occurred

‎barretenberg/cpp/src/barretenberg/nodejs_module/avm_simulate/ts_callback_utils.hpp‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,31 +27,32 @@ std::string extract_error_from_napi_value(const Napi::CallbackInfo& cb_info);
2727

2828
/**
2929
* @brief Creates a resolve handler for promises that return Buffer | undefined
30+
* @note Takes shared_ptr to ensure CallbackResults outlives the Promise handler
3031
*/
31-
Napi::Function create_buffer_resolve_handler(Napi::Env env, CallbackResults* cb_results);
32+
Napi::Function create_buffer_resolve_handler(Napi::Env env, std::shared_ptr<CallbackResults> cb_results);
3233

3334
/**
3435
* @brief Creates a resolve handler for promises that return string | undefined
36+
* @note Takes shared_ptr to ensure CallbackResults outlives the Promise handler
3537
*/
36-
Napi::Function create_string_resolve_handler(Napi::Env env, CallbackResults* cb_results);
38+
Napi::Function create_string_resolve_handler(Napi::Env env, std::shared_ptr<CallbackResults> cb_results);
3739

3840
/**
3941
* @brief Creates a resolve handler for promises that return void
42+
* @note Takes shared_ptr to ensure CallbackResults outlives the Promise handler
4043
*/
41-
Napi::Function create_void_resolve_handler(Napi::Env env, CallbackResults* cb_results);
44+
Napi::Function create_void_resolve_handler(Napi::Env env, std::shared_ptr<CallbackResults> cb_results);
4245

4346
/**
4447
* @brief Creates a reject handler for promises
48+
* @note Takes shared_ptr to ensure CallbackResults outlives the Promise handler
4549
*/
46-
Napi::Function create_reject_handler(Napi::Env env, CallbackResults* cb_results);
50+
Napi::Function create_reject_handler(Napi::Env env, std::shared_ptr<CallbackResults> cb_results);
4751

4852
/**
4953
* @brief Attaches resolve and reject handlers to a promise
5054
*/
51-
void attach_promise_handlers(Napi::Promise promise,
52-
Napi::Function resolve_handler,
53-
Napi::Function reject_handler,
54-
CallbackResults* data);
55+
void attach_promise_handlers(Napi::Promise promise, Napi::Function resolve_handler, Napi::Function reject_handler);
5556

5657
/**
5758
* @brief Serializes data to msgpack format
@@ -72,11 +73,15 @@ template <typename T> T deserialize_from_msgpack(const std::vector<uint8_t>& dat
7273
* 3. Handles promise resolution/rejection
7374
* 4. Waits with timeout
7475
* 5. Returns optional result
76+
*
77+
* @note The call_js_function receives a shared_ptr to CallbackResults. The shared_ptr MUST be
78+
* captured by the Promise handlers to ensure the CallbackResults outlives the Promise.
79+
* This prevents use-after-free when timeouts occur before the Promise resolves.
7580
*/
7681
std::optional<std::vector<uint8_t>> invoke_ts_callback_with_promise(
7782
const Napi::ThreadSafeFunction& callback,
7883
const std::string& operation_name,
79-
std::function<void(Napi::Env, Napi::Function, CallbackResults*)> call_js_function,
84+
std::function<void(Napi::Env, Napi::Function, std::shared_ptr<CallbackResults>)> call_js_function,
8085
std::chrono::seconds timeout = std::chrono::seconds(30));
8186

8287
/**

0 commit comments

Comments
 (0)