Provide custom allocators that ensure that OOM results in an abort

(closes unsafety hole)

Author: kyren

Cargo.toml

@@ -18,6 +18,9 @@ default = ["builtin-lua"]
 # assumptions about how lua is built.
 builtin-lua = ["gcc"]
 
+[dependencies]
+libc = { version = "0.2" }
+
 [build-dependencies]
 gcc = { version = "0.3", optional = true }
 

src/ffi.rs

@@ -2,17 +2,17 @@
 #![allow(non_snake_case)]
 
 use std::ptr;
-use std::os::raw::*;
+use std::os::raw::{c_void, c_char, c_int, c_longlong, c_double};
 
 pub type lua_Integer = c_longlong;
 pub type lua_Number = c_double;
 
 pub enum lua_State {}
-pub type lua_Alloc = extern "C" fn(ud: *mut c_void,
-                                   ptr: *mut c_void,
-                                   osize: usize,
-                                   nsize: usize)
-                                   -> *mut c_void;
+pub type lua_Alloc = unsafe extern "C" fn(ud: *mut c_void,
+                                          ptr: *mut c_void,
+                                          osize: usize,
+                                          nsize: usize)
+                                          -> *mut c_void;
 pub type lua_KContext = *mut c_void;
 pub type lua_KFunction = unsafe extern "C" fn(state: *mut lua_State,
                                               status: c_int,
@@ -50,6 +50,8 @@ pub const LUA_TTHREAD: c_int = 8;
 
 #[link(name = "lua5.3")]
 extern "C" {
+    pub fn lua_newstate(alloc: lua_Alloc, ud: *mut c_void) -> *mut lua_State;
+
     pub fn lua_close(state: *mut lua_State);
     pub fn lua_callk(
         state: *mut lua_State,
@@ -132,7 +134,6 @@ extern "C" {
     pub fn luaopen_debug(state: *mut lua_State) -> c_int;
     pub fn luaopen_package(state: *mut lua_State) -> c_int;
 
-    pub fn luaL_newstate() -> *mut lua_State;
     pub fn luaL_openlibs(state: *mut lua_State);
     pub fn luaL_requiref(
         state: *mut lua_State,

src/lib.rs

@@ -1,3 +1,5 @@
+extern crate libc;
+
 // Deny warnings inside doc tests / examples
 #![doc(test(attr(deny(warnings))))]
 

src/lua.rs

@@ -8,8 +8,11 @@ use std::marker::PhantomData;
 use std::collections::{HashMap, VecDeque};
 use std::collections::hash_map::Entry as HashMapEntry;
 use std::os::raw::{c_char, c_int, c_void};
+use std::process;
 use std::string::String as StdString;
 
+use libc;
+
 use ffi;
 use error::*;
 use util::*;
@@ -208,10 +211,12 @@ impl<'lua> String<'lua> {
     /// # }
     /// ```
     pub fn to_str(&self) -> Result<&str> {
-        str::from_utf8(self.as_bytes()).map_err(|e| Error::FromLuaConversionError {
-            from: "string",
-            to: "&str",
-            message: Some(e.to_string()),
+        str::from_utf8(self.as_bytes()).map_err(|e| {
+            Error::FromLuaConversionError {
+                from: "string",
+                to: "&str",
+                message: Some(e.to_string()),
+            }
         })
     }
 
@@ -1321,8 +1326,30 @@ impl Lua {
     ///
     /// Also loads the standard library.
     pub fn new() -> Lua {
+        unsafe extern "C" fn allocator(
+            _: *mut c_void,
+            ptr: *mut c_void,
+            _: usize,
+            nsize: usize,
+        ) -> *mut c_void {
+            if nsize == 0 {
+                libc::free(ptr as *mut libc::c_void);
+                ptr::null_mut()
+            } else {
+                let p = libc::realloc(ptr as *mut libc::c_void, nsize);
+                if p.is_null() {
+                    // We must abort on OOM, because otherwise this will result in an unsafe
+                    // longjmp.
+                    eprintln!("Out of memory in Lua allocation, aborting!");
+                    process::abort()
+                } else {
+                    p as *mut c_void
+                }
+            }
+        }
+
         unsafe {
-            let state = ffi::luaL_newstate();
+            let state = ffi::lua_newstate(allocator, ptr::null_mut());
 
             stack_guard(state, 0, || {
                 // Do not open the debug library, currently it can be used to cause unsafety.

src/tests.rs

@@ -70,7 +70,12 @@ fn test_eval() {
     assert_eq!(lua.eval::<i32>("return 1 + 2", None).unwrap(), 3);
     match lua.eval::<()>("if true then", None) {
         Err(Error::SyntaxError { incomplete_input: true, .. }) => {}
-        r => panic!("expected SyntaxError with incomplete_input=true, got {:?}", r),
+        r => {
+            panic!(
+                "expected SyntaxError with incomplete_input=true, got {:?}",
+                r
+            )
+        }
     }
 }
 

src/util.rs

@@ -295,11 +295,8 @@ pub unsafe fn handle_error(state: *mut ffi::lua_State, err: c_int) -> Result<()>
                     Error::RuntimeError(err_string)
                 }
                 ffi::LUA_ERRMEM => {
-                    // TODO: We should provide lua with custom allocators that guarantee aborting on
-                    // OOM, instead of relying on the system allocator.  Currently, this is a
-                    // theoretical soundness bug, because an OOM could trigger a longjmp out of
-                    // arbitrary rust code that is unprotectedly calling a lua function marked as
-                    // potentially causing memory errors, which is most of them.
+                    // This should be impossible, as we set the lua allocator to one that aborts
+                    // instead of failing.
                     eprintln!("Lua memory error, aborting!");
                     process::abort()
                 }
@@ -381,10 +378,13 @@ pub unsafe fn pcall_with_traceback(
             let traceback = CStr::from_ptr(ffi::lua_tolstring(state, -1, ptr::null_mut()))
                 .to_string_lossy()
                 .into_owned();
-            push_wrapped_error(state, Error::CallbackError {
-                traceback,
-                cause: Arc::new(error),
-            });
+            push_wrapped_error(
+                state,
+                Error::CallbackError {
+                    traceback,
+                    cause: Arc::new(error),
+                },
+            );
             ffi::lua_remove(state, -2);
         } else if !is_wrapped_panic(state, 1) {
             let s = ffi::lua_tolstring(state, 1, ptr::null_mut());
@@ -420,10 +420,13 @@ pub unsafe fn resume_with_traceback(
                 .to_str()
                 .unwrap_or_else(|_| "<could not capture traceback>")
                 .to_owned();
-            push_wrapped_error(state, Error::CallbackError {
-                traceback,
-                cause: Arc::new(error),
-            });
+            push_wrapped_error(
+                state,
+                Error::CallbackError {
+                    traceback,
+                    cause: Arc::new(error),
+                },
+            );
             ffi::lua_remove(state, -2);
         } else if !is_wrapped_panic(state, 1) {
             let s = ffi::lua_tolstring(state, 1, ptr::null_mut());