Remove static lifetime bound in OpCreator (#1276)

The main purpose of this PR is to remove the `'static` lifetime bound in

type OpCreator =
  fn(state: &Arc<IsolateState>, base: &msg::Base, data: &'static mut [u8])
    -> Box<Op>;

The reason is simple: it is plain wrong, the `data` is actually not `'static`. It is created when the message is sent from C side, and will be recycled when the message is responded. It violates the definition of `'static` lifetime.

If someone save this pointer somewhere else, and reuse it later again, uninitialized memory could be accessed. This kind of memory unsafety does not happen yet because the logic is carefully organized in this project. Lifetime constraints are maintained by code convention. It could be more robust if we can express this constraint by Rust's type system.

Basic idea: tie buffer's lifetime to an object's lifetime, a.k.a, RAII. The type `deno_buf` is pretty suitable for this job.

src/isolate.rs

@@ -38,9 +38,11 @@ pub type Buf = Box<[u8]>;
 pub type Op = Future<Item = Buf, Error = DenoError> + Send;
 
 // Returns (is_sync, op)
-pub type Dispatch =
+pub type Dispatch = fn(
-  fn(isolate: &mut Isolate, buf: &[u8], data_buf: &'static mut [u8])
+  isolate: &mut Isolate,
-    -> (bool, Box<Op>);
+  buf: libdeno::deno_buf,
+  data_buf: libdeno::deno_buf,
+) -> (bool, Box<Op>);
 
 pub struct Isolate {
   libdeno_isolate: *const libdeno::isolate,
@@ -211,12 +213,7 @@ impl Isolate {
   }
 
   fn timeout(&mut self) {
-    let dummy_buf = libdeno::deno_buf {
+    let dummy_buf = libdeno::deno_buf::empty();
-      alloc_ptr: std::ptr::null_mut(),
-      alloc_len: 0,
-      data_ptr: std::ptr::null_mut(),
-      data_len: 0,
-    };
     unsafe {
       libdeno::deno_respond(
         self.libdeno_isolate,
@@ -278,27 +275,12 @@ extern "C" fn pre_dispatch(
   data_buf: libdeno::deno_buf,
 ) {
   // for metrics
-  let bytes_sent_control = control_buf.data_len;
+  let bytes_sent_control = control_buf.len();
-  let bytes_sent_data = data_buf.data_len;
+  let bytes_sent_data = data_buf.len();
-
-  // control_buf is only valid for the lifetime of this call, thus is
-  // interpretted as a slice.
-  let control_slice = unsafe {
-    std::slice::from_raw_parts(control_buf.data_ptr, control_buf.data_len)
-  };
-
-  // data_buf is valid for the lifetime of the promise, thus a mutable buf with
-  // static lifetime.
-  let data_slice = unsafe {
-    std::slice::from_raw_parts_mut::<'static>(
-      data_buf.data_ptr,
-      data_buf.data_len,
-    )
-  };
 
   let isolate = Isolate::from_void_ptr(user_data);
   let dispatch = isolate.dispatch;
-  let (is_sync, op) = dispatch(isolate, control_slice, data_slice);
+  let (is_sync, op) = dispatch(isolate, control_buf, data_buf);
 
   isolate
     .state
@@ -391,8 +373,8 @@ mod tests {
 
   fn dispatch_sync(
     _isolate: &mut Isolate,
-    control: &[u8],
+    control: libdeno::deno_buf,
-    data: &'static mut [u8],
+    data: libdeno::deno_buf,
   ) -> (bool, Box<Op>) {
     assert_eq!(control[0], 4);
     assert_eq!(control[1], 5);
@@ -498,8 +480,8 @@ mod tests {
 
   fn metrics_dispatch_sync(
     _isolate: &mut Isolate,
-    _control: &[u8],
+    _control: libdeno::deno_buf,
-    _data: &'static mut [u8],
+    _data: libdeno::deno_buf,
   ) -> (bool, Box<Op>) {
     // Send back some sync response
     let vec: Box<[u8]> = vec![1, 2, 3, 4].into_boxed_slice();
@@ -509,8 +491,8 @@ mod tests {
 
   fn metrics_dispatch_async(
     _isolate: &mut Isolate,
-    _control: &[u8],
+    _control: libdeno::deno_buf,
-    _data: &'static mut [u8],
+    _data: libdeno::deno_buf,
   ) -> (bool, Box<Op>) {
     // Send back some sync response
     let vec: Box<[u8]> = vec![1, 2, 3, 4].into_boxed_slice();

src/libdeno.rs

@@ -2,45 +2,93 @@
 use libc::c_char;
 use libc::c_int;
 use libc::c_void;
-use std::ptr::null_mut;
+use std::ops::{Deref, DerefMut};
+use std::ptr::null;
 
+// TODO(F001): change this definition to `extern { pub type isolate; }`
+// After RFC 1861 is stablized. See https://github.com/rust-lang/rust/issues/43467.
 #[repr(C)]
 pub struct isolate {
   _unused: [u8; 0],
 }
 
+/// If "alloc_ptr" is not null, this type represents a buffer which is created
+/// in C side, and then passed to Rust side by `DenoRecvCb`. Finally it should
+/// be moved back to C side by `deno_respond`. If it is not passed to
+/// `deno_respond` in the end, it will be leaked.
+///
+/// If "alloc_ptr" is null, this type represents a borrowed slice.
 #[repr(C)]
-#[derive(Clone, PartialEq)]
 pub struct deno_buf {
-  pub alloc_ptr: *mut u8,
+  alloc_ptr: *const u8,
-  pub alloc_len: usize,
+  alloc_len: usize,
-  pub data_ptr: *mut u8,
+  data_ptr: *const u8,
-  pub data_len: usize,
+  data_len: usize,
 }
 
+/// `deno_buf` can not clone, and there is no interior mutability.
+/// This type satisfies Send bound.
+unsafe impl Send for deno_buf {}
+
 impl deno_buf {
   pub fn empty() -> Self {
     Self {
-      alloc_ptr: null_mut(),
+      alloc_ptr: null(),
       alloc_len: 0,
-      data_ptr: null_mut(),
+      data_ptr: null(),
       data_len: 0,
     }
   }
+
+  pub unsafe fn from_raw_parts(ptr: *const u8, len: usize) -> Self {
+    Self {
+      alloc_ptr: null(),
+      alloc_len: 0,
+      data_ptr: ptr,
+      data_len: len,
+    }
+  }
 }
 
 /// Converts Rust &Buf to libdeno `deno_buf`.
 impl<'a> From<&'a [u8]> for deno_buf {
   fn from(x: &'a [u8]) -> Self {
     Self {
-      alloc_ptr: std::ptr::null_mut(),
+      alloc_ptr: null(),
       alloc_len: 0,
-      data_ptr: x.as_ref().as_ptr() as *mut u8,
+      data_ptr: x.as_ref().as_ptr(),
       data_len: x.len(),
     }
   }
 }
 
+impl Deref for deno_buf {
+  type Target = [u8];
+  fn deref(&self) -> &[u8] {
+    unsafe { std::slice::from_raw_parts(self.data_ptr, self.data_len) }
+  }
+}
+
+impl DerefMut for deno_buf {
+  fn deref_mut(&mut self) -> &mut [u8] {
+    unsafe {
+      std::slice::from_raw_parts_mut(self.data_ptr as *mut u8, self.data_len)
+    }
+  }
+}
+
+impl AsRef<[u8]> for deno_buf {
+  fn as_ref(&self) -> &[u8] {
+    &*self
+  }
+}
+
+impl AsMut<[u8]> for deno_buf {
+  fn as_mut(&mut self) -> &mut [u8] {
+    &mut *self
+  }
+}
+
 type DenoRecvCb = unsafe extern "C" fn(
   user_data: *mut c_void,
   req_id: i32,

src/ops.rs

@@ -7,6 +7,7 @@ use isolate::Buf;
 use isolate::Isolate;
 use isolate::IsolateState;
 use isolate::Op;
+use libdeno;
 use msg;
 use msg_util;
 use resources;
@@ -48,7 +49,7 @@ type OpResult = DenoResult<Buf>;
 // TODO Ideally we wouldn't have to box the Op being returned.
 // The box is just to make it easier to get a prototype refactor working.
 type OpCreator =
-  fn(state: &Arc<IsolateState>, base: &msg::Base, data: &'static mut [u8])
+  fn(state: &Arc<IsolateState>, base: &msg::Base, data: libdeno::deno_buf)
     -> Box<Op>;
 
 // Hopefully Rust optimizes this away.
@@ -62,10 +63,10 @@ fn empty_buf() -> Buf {
 /// data corresponds to the second argument of libdeno.send().
 pub fn dispatch(
   isolate: &mut Isolate,
-  control: &[u8],
+  control: libdeno::deno_buf,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> (bool, Box<Op>) {
-  let base = msg::get_root_as_base(control);
+  let base = msg::get_root_as_base(&control);
   let is_sync = base.sync();
   let inner_type = base.inner_type();
   let cmd_id = base.cmd_id();
@@ -170,7 +171,7 @@ pub fn dispatch(
 fn op_exit(
   _config: &Arc<IsolateState>,
   base: &msg::Base,
-  _data: &'static mut [u8],
+  _data: libdeno::deno_buf,
 ) -> Box<Op> {
   let inner = base.inner_as_exit().unwrap();
   std::process::exit(inner.code())
@@ -179,7 +180,7 @@ fn op_exit(
 fn op_start(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let mut builder = FlatBufferBuilder::new();
@@ -250,7 +251,7 @@ fn odd_future(err: DenoError) -> Box<Op> {
 fn op_code_fetch(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_code_fetch().unwrap();
@@ -293,7 +294,7 @@ fn op_code_fetch(
 fn op_code_cache(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_code_cache().unwrap();
@@ -312,7 +313,7 @@ fn op_code_cache(
 fn op_chdir(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_chdir().unwrap();
@@ -326,7 +327,7 @@ fn op_chdir(
 fn op_set_timeout(
   isolate: &mut Isolate,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_set_timeout().unwrap();
@@ -343,7 +344,7 @@ fn op_set_timeout(
 fn op_set_env(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_set_env().unwrap();
@@ -359,7 +360,7 @@ fn op_set_env(
 fn op_env(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let cmd_id = base.cmd_id();
@@ -391,7 +392,7 @@ fn op_env(
 fn op_fetch(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   let inner = base.inner_as_fetch().unwrap();
   let cmd_id = base.cmd_id();
@@ -403,7 +404,7 @@ fn op_fetch(
   let body = if data.is_empty() {
     hyper::Body::empty()
   } else {
-    hyper::Body::from(Vec::from(data))
+    hyper::Body::from(Vec::from(&*data))
   };
 
   let req = msg_util::deserialize_request(header, body);
@@ -480,7 +481,7 @@ macro_rules! blocking {
 fn op_make_temp_dir(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let base = Box::new(*base);
@@ -529,7 +530,7 @@ fn op_make_temp_dir(
 fn op_mkdir(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_mkdir().unwrap();
@@ -549,7 +550,7 @@ fn op_mkdir(
 fn op_chmod(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_chmod().unwrap();
@@ -582,7 +583,7 @@ fn op_chmod(
 fn op_open(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let cmd_id = base.cmd_id();
@@ -613,7 +614,7 @@ fn op_open(
 fn op_close(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_close().unwrap();
@@ -630,7 +631,7 @@ fn op_close(
 fn op_shutdown(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_shutdown().unwrap();
@@ -656,7 +657,7 @@ fn op_shutdown(
 fn op_read(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   let cmd_id = base.cmd_id();
   let inner = base.inner_as_read().unwrap();
@@ -694,7 +695,7 @@ fn op_read(
 fn op_write(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   let cmd_id = base.cmd_id();
   let inner = base.inner_as_write().unwrap();
@@ -731,7 +732,7 @@ fn op_write(
 fn op_remove(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_remove().unwrap();
@@ -761,7 +762,7 @@ fn op_remove(
 fn op_read_file(
   _config: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_read_file().unwrap();
@@ -795,7 +796,7 @@ fn op_read_file(
 fn op_copy_file(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_copy_file().unwrap();
@@ -847,7 +848,7 @@ fn get_mode(_perm: &fs::Permissions) -> u32 {
 fn op_cwd(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let cmd_id = base.cmd_id();
@@ -873,7 +874,7 @@ fn op_cwd(
 fn op_stat(
   _config: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_stat().unwrap();
@@ -920,7 +921,7 @@ fn op_stat(
 fn op_read_dir(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_read_dir().unwrap();
@@ -976,7 +977,7 @@ fn op_read_dir(
 fn op_write_file(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   let inner = base.inner_as_write_file().unwrap();
   let filename = String::from(inner.filename().unwrap());
@@ -988,7 +989,7 @@ fn op_write_file(
 
   blocking!(base.sync(), || -> OpResult {
     debug!("op_write_file {} {}", filename, data.len());
-    deno_fs::write_file(Path::new(&filename), data, perm)?;
+    deno_fs::write_file(Path::new(&filename), &data, perm)?;
     Ok(empty_buf())
   })
 }
@@ -996,7 +997,7 @@ fn op_write_file(
 fn op_rename(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_rename().unwrap();
@@ -1016,7 +1017,7 @@ fn op_rename(
 fn op_symlink(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_symlink().unwrap();
@@ -1045,7 +1046,7 @@ fn op_symlink(
 fn op_read_link(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_readlink().unwrap();
@@ -1078,7 +1079,7 @@ fn op_read_link(
 fn op_repl_start(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_repl_start().unwrap();
@@ -1109,7 +1110,7 @@ fn op_repl_start(
 fn op_repl_readline(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let inner = base.inner_as_repl_readline().unwrap();
@@ -1147,7 +1148,7 @@ fn op_repl_readline(
 fn op_truncate(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
 
@@ -1170,7 +1171,7 @@ fn op_truncate(
 fn op_listen(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   if let Err(e) = state.check_net("listen") {
@@ -1236,7 +1237,7 @@ fn new_conn(cmd_id: u32, tcp_stream: TcpStream) -> OpResult {
 fn op_accept(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   if let Err(e) = state.check_net("accept") {
@@ -1262,7 +1263,7 @@ fn op_accept(
 fn op_dial(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   if let Err(e) = state.check_net("dial") {
@@ -1286,7 +1287,7 @@ fn op_dial(
 fn op_metrics(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let cmd_id = base.cmd_id();
@@ -1310,7 +1311,7 @@ fn op_metrics(
 fn op_resources(
   _state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let cmd_id = base.cmd_id();
@@ -1362,7 +1363,7 @@ fn subprocess_stdio_map(v: msg::ProcessStdio) -> std::process::Stdio {
 fn op_run(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert!(base.sync());
   let cmd_id = base.cmd_id();
@@ -1430,7 +1431,7 @@ fn op_run(
 fn op_run_status(
   state: &Arc<IsolateState>,
   base: &msg::Base,
-  data: &'static mut [u8],
+  data: libdeno::deno_buf,
 ) -> Box<Op> {
   assert_eq!(data.len(), 0);
   let cmd_id = base.cmd_id();

src/snapshot.rs

@@ -1,19 +1,9 @@
 // Copyright 2018 the Deno authors. All rights reserved. MIT license.
 use libdeno::deno_buf;
-use std;
 
 pub fn deno_snapshot() -> deno_buf {
   let data =
     include_bytes!(concat!(env!("GN_OUT_DIR"), "/gen/snapshot_deno.bin"));
-  let ptr = data.as_ptr();
+
-  // TODO The cast is not necessary here. deno_buf specifies mutable
+  unsafe { deno_buf::from_raw_parts(data.as_ptr(), data.len()) }
-  // pointers when it doesn't necessarally need mutable. So maybe the deno_buf
-  // type should be broken into a mutable and non-mutable version?
-  let ptr_mut = ptr as *mut u8;
-  deno_buf {
-    alloc_ptr: std::ptr::null_mut(),
-    alloc_len: 0,
-    data_ptr: ptr_mut,
-    data_len: data.len(),
-  }
 }