From e742af10aa67dc90a68fcb60b8e70733fd753e68 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Sun, 7 Oct 2018 23:48:06 +0200 Subject: [PATCH] Abide by the rules when passing Isolate between c and rust Ensure that at most one mutable Isolate reference exists at a time. `deno_execute()` and `deno_respond()` now borrow a reference to the rust-side isolate from the caller. When we need a reference to the isolate while one of these functions is on the stack, `deno_get_data()` can be used to borrow back that reference. --- Roadmap.md | 2 +- libdeno/binding.cc | 31 ++++++-- libdeno/deno.h | 7 +- libdeno/from_filesystem.cc | 8 +-- libdeno/from_snapshot.cc | 8 +-- libdeno/libdeno_test.cc | 140 ++++++++++++++++++------------------- src/isolate.rs | 71 ++++++++----------- src/libdeno.rs | 10 ++- 8 files changed, 145 insertions(+), 132 deletions(-) diff --git a/Roadmap.md b/Roadmap.md index e310b9a5..ff0f7007 100644 --- a/Roadmap.md +++ b/Roadmap.md @@ -131,7 +131,7 @@ void deno_set_callback(Deno* deno, deno_sub_cb cb); // Get error text with deno_last_exception(). // 0 = success, non-zero = failure. // TODO(ry) Currently the return code has opposite semantics. -int deno_execute(Deno* d, const char* js_filename, const char* js_source); +int deno_execute(Deno* d, void* user_data, const char* js_filename, const char* js_source); // This call doesn't go into JS. This is thread-safe. // TODO(ry) Currently this is called deno_pub. It should be renamed. diff --git a/libdeno/binding.cc b/libdeno/binding.cc index 7bc1a039..55a78069 100644 --- a/libdeno/binding.cc +++ b/libdeno/binding.cc @@ -401,6 +401,24 @@ void AddIsolate(Deno* d, v8::Isolate* isolate) { d->isolate->SetData(0, d); } +class UserDataScope { + Deno* deno; + void* prev_data; + void* data; // Not necessary; only for sanity checking. + + public: + UserDataScope(Deno* deno_, void* data_) : deno(deno_), data(data_) { + CHECK(deno->user_data == nullptr || deno->user_data == data_); + prev_data = deno->user_data; + deno->user_data = data; + } + + ~UserDataScope() { + CHECK(deno->user_data == data); + deno->user_data = prev_data; + } +}; + } // namespace deno extern "C" { @@ -413,7 +431,10 @@ void deno_init() { v8::V8::Initialize(); } -void* deno_get_data(Deno* d) { return d->user_data; } +void* deno_get_data(const Deno* d) { + CHECK(d->user_data != nullptr); + return d->user_data; +} const char* deno_v8_version() { return v8::V8::GetVersion(); } @@ -423,7 +444,9 @@ void deno_set_v8_flags(int* argc, char** argv) { const char* deno_last_exception(Deno* d) { return d->last_exception.c_str(); } -int deno_execute(Deno* d, const char* js_filename, const char* js_source) { +int deno_execute(Deno* d, void* user_data, const char* js_filename, + const char* js_source) { + deno::UserDataScope user_data_scope(d, user_data); auto* isolate = d->isolate; v8::Locker locker(isolate); v8::Isolate::Scope isolate_scope(isolate); @@ -432,7 +455,7 @@ int deno_execute(Deno* d, const char* js_filename, const char* js_source) { return deno::Execute(context, js_filename, js_source) ? 1 : 0; } -int deno_respond(Deno* d, int32_t req_id, deno_buf buf) { +int deno_respond(Deno* d, void* user_data, int32_t req_id, deno_buf buf) { if (d->currentArgs != nullptr) { // Synchronous response. auto ab = deno::ImportBuf(d->isolate, buf); @@ -442,7 +465,7 @@ int deno_respond(Deno* d, int32_t req_id, deno_buf buf) { } // Asynchronous response. - + deno::UserDataScope user_data_scope(d, user_data); v8::Locker locker(d->isolate); v8::Isolate::Scope isolate_scope(d->isolate); v8::HandleScope handle_scope(d->isolate); diff --git a/libdeno/deno.h b/libdeno/deno.h index d3c2cf76..c166c7fd 100644 --- a/libdeno/deno.h +++ b/libdeno/deno.h @@ -30,7 +30,7 @@ void deno_init(); const char* deno_v8_version(); void deno_set_v8_flags(int* argc, char** argv); -Deno* deno_new(void* user_data, deno_recv_cb cb); +Deno* deno_new(deno_recv_cb cb); void deno_delete(Deno* d); // Returns the void* user_data provided in deno_new. @@ -39,7 +39,8 @@ void* deno_get_data(Deno*); // Returns false on error. // Get error text with deno_last_exception(). // 0 = fail, 1 = success -int deno_execute(Deno* d, const char* js_filename, const char* js_source); +int deno_execute(Deno* d, void* user_data, const char* js_filename, + const char* js_source); // deno_respond sends up to one message back for every deno_recv_cb made. // @@ -59,7 +60,7 @@ int deno_execute(Deno* d, const char* js_filename, const char* js_source); // // A non-zero return value, means a JS exception was encountered during the // libdeno.recv() callback. Check deno_last_exception() for exception text. -int deno_respond(Deno* d, int32_t req_id, deno_buf buf); +int deno_respond(Deno* d, void* user_data, int32_t req_id, deno_buf buf); const char* deno_last_exception(Deno* d); diff --git a/libdeno/from_filesystem.cc b/libdeno/from_filesystem.cc index 701925c9..6b9f5ca1 100644 --- a/libdeno/from_filesystem.cc +++ b/libdeno/from_filesystem.cc @@ -14,7 +14,7 @@ namespace deno { -Deno* NewFromFileSystem(void* user_data, deno_recv_cb cb) { +Deno* NewFromFileSystem(deno_recv_cb cb) { std::string exe_path; CHECK(deno::ExePath(&exe_path)); std::string exe_dir = deno::Dirname(exe_path); // Always ends with a slash. @@ -30,7 +30,7 @@ Deno* NewFromFileSystem(void* user_data, deno_recv_cb cb) { Deno* d = new Deno; d->currentArgs = nullptr; d->cb = cb; - d->user_data = user_data; + d->user_data = nullptr; v8::Isolate::CreateParams params; params.array_buffer_allocator = v8::ArrayBuffer::Allocator::NewDefaultAllocator(); @@ -55,7 +55,5 @@ Deno* NewFromFileSystem(void* user_data, deno_recv_cb cb) { } // namespace deno extern "C" { -Deno* deno_new(void* user_data, deno_recv_cb cb) { - return deno::NewFromFileSystem(user_data, cb); -} +Deno* deno_new(deno_recv_cb cb) { return deno::NewFromFileSystem(cb); } } diff --git a/libdeno/from_snapshot.cc b/libdeno/from_snapshot.cc index b204af08..1280f245 100644 --- a/libdeno/from_snapshot.cc +++ b/libdeno/from_snapshot.cc @@ -43,11 +43,11 @@ void DeserializeInternalFields(v8::Local holder, int index, deserialized_data.push_back(embedder_field); } -Deno* NewFromSnapshot(void* user_data, deno_recv_cb cb) { +Deno* NewFromSnapshot(deno_recv_cb cb) { Deno* d = new Deno; d->currentArgs = nullptr; d->cb = cb; - d->user_data = user_data; + d->user_data = nullptr; v8::Isolate::CreateParams params; params.array_buffer_allocator = v8::ArrayBuffer::Allocator::NewDefaultAllocator(); @@ -80,7 +80,5 @@ Deno* NewFromSnapshot(void* user_data, deno_recv_cb cb) { } // namespace deno extern "C" { -Deno* deno_new(void* user_data, deno_recv_cb cb) { - return deno::NewFromSnapshot(user_data, cb); -} +Deno* deno_new(deno_recv_cb cb) { return deno::NewFromSnapshot(cb); } } diff --git a/libdeno/libdeno_test.cc b/libdeno/libdeno_test.cc index 5177ab4a..2748a60c 100644 --- a/libdeno/libdeno_test.cc +++ b/libdeno/libdeno_test.cc @@ -4,21 +4,21 @@ #include "deno.h" TEST(LibDenoTest, InitializesCorrectly) { - Deno* d = deno_new(nullptr, nullptr); - EXPECT_TRUE(deno_execute(d, "a.js", "1 + 2")); + Deno* d = deno_new(nullptr); + EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "1 + 2")); deno_delete(d); } TEST(LibDenoTest, CanCallFunction) { - Deno* d = deno_new(nullptr, nullptr); - EXPECT_TRUE(deno_execute(d, "a.js", + Deno* d = deno_new(nullptr); + EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "if (CanCallFunction() != 'foo') throw Error();")); deno_delete(d); } TEST(LibDenoTest, ErrorsCorrectly) { - Deno* d = deno_new(nullptr, nullptr); - EXPECT_FALSE(deno_execute(d, "a.js", "throw Error()")); + Deno* d = deno_new(nullptr); + EXPECT_FALSE(deno_execute(d, nullptr, "a.js", "throw Error()")); deno_delete(d); } @@ -54,7 +54,7 @@ void assert_null(deno_buf b) { TEST(LibDenoTest, RecvReturnEmpty) { static int count = 0; - Deno* d = deno_new(nullptr, [](auto _, int req_id, auto buf, auto data_buf) { + Deno* d = deno_new([](auto _, int req_id, auto buf, auto data_buf) { assert_null(data_buf); count++; EXPECT_EQ(static_cast(3), buf.data_len); @@ -62,71 +62,69 @@ TEST(LibDenoTest, RecvReturnEmpty) { EXPECT_EQ(buf.data_ptr[1], 'b'); EXPECT_EQ(buf.data_ptr[2], 'c'); }); - EXPECT_TRUE(deno_execute(d, "a.js", "RecvReturnEmpty()")); + EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "RecvReturnEmpty()")); EXPECT_EQ(count, 2); deno_delete(d); } TEST(LibDenoTest, RecvReturnBar) { static int count = 0; - Deno* d = - deno_new(nullptr, [](auto deno, int req_id, auto buf, auto data_buf) { - assert_null(data_buf); - count++; - EXPECT_EQ(static_cast(3), buf.data_len); - EXPECT_EQ(buf.data_ptr[0], 'a'); - EXPECT_EQ(buf.data_ptr[1], 'b'); - EXPECT_EQ(buf.data_ptr[2], 'c'); - deno_respond(deno, req_id, strbuf("bar")); - }); - EXPECT_TRUE(deno_execute(d, "a.js", "RecvReturnBar()")); + Deno* d = deno_new([](auto deno, int req_id, auto buf, auto data_buf) { + assert_null(data_buf); + count++; + EXPECT_EQ(static_cast(3), buf.data_len); + EXPECT_EQ(buf.data_ptr[0], 'a'); + EXPECT_EQ(buf.data_ptr[1], 'b'); + EXPECT_EQ(buf.data_ptr[2], 'c'); + deno_respond(deno, nullptr, req_id, strbuf("bar")); + }); + EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "RecvReturnBar()")); EXPECT_EQ(count, 1); deno_delete(d); } TEST(LibDenoTest, DoubleRecvFails) { - Deno* d = deno_new(nullptr, nullptr); - EXPECT_FALSE(deno_execute(d, "a.js", "DoubleRecvFails()")); + Deno* d = deno_new(nullptr); + EXPECT_FALSE(deno_execute(d, nullptr, "a.js", "DoubleRecvFails()")); deno_delete(d); } TEST(LibDenoTest, SendRecvSlice) { static int count = 0; - Deno* d = - deno_new(nullptr, [](auto deno, int req_id, auto buf, auto data_buf) { - assert_null(data_buf); - static const size_t alloc_len = 1024; - size_t i = count++; - // Check the size and offset of the slice. - size_t data_offset = buf.data_ptr - buf.alloc_ptr; - EXPECT_EQ(data_offset, i * 11); - EXPECT_EQ(buf.data_len, alloc_len - i * 30); - EXPECT_EQ(buf.alloc_len, alloc_len); - // Check values written by the JS side. - EXPECT_EQ(buf.data_ptr[0], 100 + i); - EXPECT_EQ(buf.data_ptr[buf.data_len - 1], 100 - i); - // Make copy of the backing buffer -- this is currently necessary - // because deno_respond() takes ownership over the buffer, but we are - // not given ownership of `buf` by our caller. - uint8_t* alloc_ptr = reinterpret_cast(malloc(alloc_len)); - memcpy(alloc_ptr, buf.alloc_ptr, alloc_len); - // Make a slice that is a bit shorter than the original. - deno_buf buf2{alloc_ptr, alloc_len, alloc_ptr + data_offset, - buf.data_len - 19}; - // Place some values into the buffer for the JS side to verify. - buf2.data_ptr[0] = 200 + i; - buf2.data_ptr[buf2.data_len - 1] = 200 - i; - // Send back. - deno_respond(deno, req_id, buf2); - }); - EXPECT_TRUE(deno_execute(d, "a.js", "SendRecvSlice()")); + Deno* d = deno_new([](auto deno, int req_id, auto buf, auto data_buf) { + assert_null(data_buf); + static const size_t alloc_len = 1024; + size_t i = count++; + // Check the size and offset of the slice. + size_t data_offset = buf.data_ptr - buf.alloc_ptr; + EXPECT_EQ(data_offset, i * 11); + EXPECT_EQ(buf.data_len, alloc_len - i * 30); + EXPECT_EQ(buf.alloc_len, alloc_len); + // Check values written by the JS side. + EXPECT_EQ(buf.data_ptr[0], 100 + i); + EXPECT_EQ(buf.data_ptr[buf.data_len - 1], 100 - i); + // Make copy of the backing buffer -- this is currently necessary + // because deno_respond() takes ownership over the buffer, but we are + // not given ownership of `buf` by our caller. + uint8_t* alloc_ptr = reinterpret_cast(malloc(alloc_len)); + memcpy(alloc_ptr, buf.alloc_ptr, alloc_len); + // Make a slice that is a bit shorter than the original. + deno_buf buf2{alloc_ptr, alloc_len, alloc_ptr + data_offset, + buf.data_len - 19}; + // Place some values into the buffer for the JS side to verify. + buf2.data_ptr[0] = 200 + i; + buf2.data_ptr[buf2.data_len - 1] = 200 - i; + // Send back. + deno_respond(deno, nullptr, req_id, buf2); + }); + EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "SendRecvSlice()")); EXPECT_EQ(count, 5); deno_delete(d); } TEST(LibDenoTest, JSSendArrayBufferViewTypes) { static int count = 0; - Deno* d = deno_new(nullptr, [](auto _, int req_id, auto buf, auto data_buf) { + Deno* d = deno_new([](auto _, int req_id, auto buf, auto data_buf) { assert_null(data_buf); count++; size_t data_offset = buf.data_ptr - buf.alloc_ptr; @@ -135,57 +133,57 @@ TEST(LibDenoTest, JSSendArrayBufferViewTypes) { EXPECT_EQ(buf.alloc_len, 4321u); EXPECT_EQ(buf.data_ptr[0], count); }); - EXPECT_TRUE(deno_execute(d, "a.js", "JSSendArrayBufferViewTypes()")); + EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "JSSendArrayBufferViewTypes()")); EXPECT_EQ(count, 3); deno_delete(d); } TEST(LibDenoTest, TypedArraySnapshots) { - Deno* d = deno_new(nullptr, nullptr); - EXPECT_TRUE(deno_execute(d, "a.js", "TypedArraySnapshots()")); + Deno* d = deno_new(nullptr); + EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "TypedArraySnapshots()")); deno_delete(d); } TEST(LibDenoTest, SnapshotBug) { - Deno* d = deno_new(nullptr, nullptr); - EXPECT_TRUE(deno_execute(d, "a.js", "SnapshotBug()")); + Deno* d = deno_new(nullptr); + EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "SnapshotBug()")); deno_delete(d); } TEST(LibDenoTest, GlobalErrorHandling) { static int count = 0; - Deno* d = deno_new(nullptr, [](auto _, int req_id, auto buf, auto data_buf) { + Deno* d = deno_new([](auto _, int req_id, auto buf, auto data_buf) { assert_null(data_buf); count++; EXPECT_EQ(static_cast(1), buf.data_len); EXPECT_EQ(buf.data_ptr[0], 42); }); - EXPECT_FALSE(deno_execute(d, "a.js", "GlobalErrorHandling()")); + EXPECT_FALSE(deno_execute(d, nullptr, "a.js", "GlobalErrorHandling()")); EXPECT_EQ(count, 1); deno_delete(d); } TEST(LibDenoTest, DoubleGlobalErrorHandlingFails) { - Deno* d = deno_new(nullptr, nullptr); - EXPECT_FALSE(deno_execute(d, "a.js", "DoubleGlobalErrorHandlingFails()")); + Deno* d = deno_new(nullptr); + EXPECT_FALSE( + deno_execute(d, nullptr, "a.js", "DoubleGlobalErrorHandlingFails()")); deno_delete(d); } TEST(LibDenoTest, DataBuf) { static int count = 0; static deno_buf data_buf_copy; - Deno* d = deno_new(nullptr, - [](auto _, int req_id, deno_buf buf, deno_buf data_buf) { - count++; - data_buf.data_ptr[0] = 4; - data_buf.data_ptr[1] = 2; - data_buf_copy = data_buf; - EXPECT_EQ(2u, buf.data_len); - EXPECT_EQ(2u, data_buf.data_len); - EXPECT_EQ(buf.data_ptr[0], 1); - EXPECT_EQ(buf.data_ptr[1], 2); - }); - EXPECT_TRUE(deno_execute(d, "a.js", "DataBuf()")); + Deno* d = deno_new([](auto _, int req_id, deno_buf buf, deno_buf data_buf) { + count++; + data_buf.data_ptr[0] = 4; + data_buf.data_ptr[1] = 2; + data_buf_copy = data_buf; + EXPECT_EQ(2u, buf.data_len); + EXPECT_EQ(2u, data_buf.data_len); + EXPECT_EQ(buf.data_ptr[0], 1); + EXPECT_EQ(buf.data_ptr[1], 2); + }); + EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "DataBuf()")); EXPECT_EQ(count, 1); // data_buf was subsequently changed in JS, let's check that our copy reflects // that. diff --git a/src/isolate.rs b/src/isolate.rs index 797af80d..8f8e798f 100644 --- a/src/isolate.rs +++ b/src/isolate.rs @@ -14,7 +14,6 @@ use libc::c_void; use std; use std::ffi::CStr; use std::ffi::CString; -use std::sync::atomic; use std::sync::mpsc; use std::sync::Arc; use std::sync::Mutex; @@ -72,18 +71,18 @@ impl IsolateState { static DENO_INIT: std::sync::Once = std::sync::ONCE_INIT; impl Isolate { - pub fn new(argv: Vec, dispatch: Dispatch) -> Box { + pub fn new(argv: Vec, dispatch: Dispatch) -> Isolate { DENO_INIT.call_once(|| { unsafe { libdeno::deno_init() }; }); let (flags, argv_rest) = flags::set_flags(argv); - + let libdeno_isolate = unsafe { libdeno::deno_new(pre_dispatch) }; // This channel handles sending async messages back to the runtime. let (tx, rx) = mpsc::channel::<(i32, Buf)>(); - let mut isolate = Box::new(Isolate { - libdeno_isolate: 0 as *const libdeno::isolate, + Isolate { + libdeno_isolate, dispatch, rx, ntasks: 0, @@ -94,24 +93,20 @@ impl Isolate { flags, tx: Mutex::new(Some(tx)), }), - }); - - (*isolate).libdeno_isolate = unsafe { - libdeno::deno_new(isolate.as_mut() as *mut _ as *mut c_void, pre_dispatch) - }; + } + } - isolate + pub fn as_void_ptr(&mut self) -> *mut c_void { + self as *mut _ as *mut c_void } pub fn from_c<'a>(d: *const libdeno::isolate) -> &'a mut Isolate { - let ptr = unsafe { libdeno::deno_get_data(d) }; - let ptr = ptr as *mut Isolate; - let isolate_box = unsafe { Box::from_raw(ptr) }; - Box::leak(isolate_box) + let ptr = unsafe { libdeno::deno_get_data(d) } as *mut _; + unsafe { &mut *ptr } } pub fn execute( - &self, + &mut self, js_filename: &str, js_source: &str, ) -> Result<(), DenoException> { @@ -120,6 +115,7 @@ impl Isolate { let r = unsafe { libdeno::deno_execute( self.libdeno_isolate, + self.as_void_ptr(), filename.as_ptr(), source.as_ptr(), ) @@ -132,10 +128,17 @@ impl Isolate { Ok(()) } - pub fn respond(&self, req_id: i32, buf: Buf) { + pub fn respond(&mut self, req_id: i32, buf: Buf) { // TODO(zero-copy) Use Buf::leak(buf) to leak the heap allocated buf. And // don't do the memcpy in ImportBuf() (in libdeno/binding.cc) - unsafe { libdeno::deno_respond(self.libdeno_isolate, req_id, buf.into()) } + unsafe { + libdeno::deno_respond( + self.libdeno_isolate, + self.as_void_ptr(), + req_id, + buf.into(), + ) + } } fn complete_op(&mut self, req_id: i32, buf: Buf) { @@ -146,14 +149,21 @@ impl Isolate { self.respond(req_id, buf); } - fn timeout(&self) { + fn timeout(&mut self) { let dummy_buf = libdeno::deno_buf { alloc_ptr: 0 as *mut u8, alloc_len: 0, data_ptr: 0 as *mut u8, data_len: 0, }; - unsafe { libdeno::deno_respond(self.libdeno_isolate, -1, dummy_buf) } + unsafe { + libdeno::deno_respond( + self.libdeno_isolate, + self.as_void_ptr(), + -1, + dummy_buf, + ) + } } // TODO Use Park abstraction? Note at time of writing Tokio default runtime @@ -280,27 +290,6 @@ mod tests { use super::*; use futures; - #[test] - fn test_c_to_rust() { - let argv = vec![String::from("./deno"), String::from("hello.js")]; - let isolate = Isolate::new(argv, unreachable_dispatch); - let isolate2 = Isolate::from_c(isolate.libdeno_isolate); - assert_eq!(isolate.libdeno_isolate, isolate2.libdeno_isolate); - assert_eq!( - isolate.state.dir.root.join("gen"), - isolate.state.dir.gen, - "Sanity check" - ); - } - - fn unreachable_dispatch( - _isolate: &mut Isolate, - _control: &[u8], - _data: &'static mut [u8], - ) -> (bool, Box) { - unreachable!(); - } - #[test] fn test_dispatch_sync() { let argv = vec![String::from("./deno"), String::from("hello.js")]; diff --git a/src/libdeno.rs b/src/libdeno.rs index b48ddd16..77f7f5c3 100644 --- a/src/libdeno.rs +++ b/src/libdeno.rs @@ -30,13 +30,19 @@ extern "C" { pub fn deno_init(); pub fn deno_v8_version() -> *const c_char; pub fn deno_set_v8_flags(argc: *mut c_int, argv: *mut *mut c_char); - pub fn deno_new(user_data: *mut c_void, cb: DenoRecvCb) -> *const isolate; + pub fn deno_new(cb: DenoRecvCb) -> *const isolate; pub fn deno_delete(i: *const isolate); pub fn deno_last_exception(i: *const isolate) -> *const c_char; pub fn deno_get_data(i: *const isolate) -> *mut c_void; - pub fn deno_respond(i: *const isolate, req_id: i32, buf: deno_buf); + pub fn deno_respond( + i: *const isolate, + user_data: *mut c_void, + req_id: i32, + buf: deno_buf, + ); pub fn deno_execute( i: *const isolate, + user_data: *mut c_void, js_filename: *const c_char, js_source: *const c_char, ) -> c_int; -- GitLab