v0.8.8

If no free registry id found, we call protect_lua! macro while keeping mutex guard to the unref list.
Protected calls can trigger garbage collection and if RegistryKey is placed in userdata being collected, this can lead to deadlock.
The solution is drop mutex guard as soon as possible.
Also this commit includes optimization in creating reference in Lua registry.

CHANGELOG.md

@@ -1,3 +1,19 @@
+## v0.8.8
+
+- Fix potential deadlock when trying to reuse dropped registry keys.
+- Optimize userdata methods call when __index and fields_getters are nil
+
+## v0.8.7
+
+- Minimum Luau updated to 0.555 (`LUAI_MAXCSTACK` limit increased to 100000)
+- `_VERSION` in Luau now includes version number
+- Fixed lifetime of `DebugNames` in `Debug::names()` and `DebugSource` in `Debug::source()`
+- Fixed subtraction overflow when calculating index for `MultiValue::get()`
+
+## v0.8.6
+
+- Fixed bug when recycled Registry slot can be set to Nil
+
 ## v0.8.5
 
 - Fixed potential unsoundness when using `Layout::from_size_align_unchecked` and Rust 1.65+

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "mlua"
-version = "0.8.5" # remember to update html_root_url and mlua_derive
+version = "0.8.8" # remember to update html_root_url and mlua_derive
 authors = ["Aleksandr Orlenko <zxteam@pm.me>", "kyren <catherine@chucklefish.org>"]
 edition = "2021"
 repository = "https://github.com/khvzak/mlua"
@@ -58,7 +58,7 @@ cc = { version = "1.0" }
 pkg-config = { version = "0.3.17" }
 lua-src = { version = ">= 544.0.0, < 550.0.0", optional = true }
 luajit-src = { version = ">= 210.4.0, < 220.0.0", optional = true }
-luau0-src = { version = "0.4.0", optional = true }
+luau0-src = { version = "0.5.0", optional = true }
 
 [dev-dependencies]
 rustyline = "10.0"

README.md

@@ -7,7 +7,7 @@
 [crates.io]: https://crates.io/crates/mlua
 [API Documentation]: https://docs.rs/mlua/badge.svg
 [docs.rs]: https://docs.rs/mlua
-[Coverage Status]: https://codecov.io/gh/khvzak/mlua/branch/master/graph/badge.svg?token=99339FS1CG
+[Coverage Status]: https://codecov.io/gh/khvzak/mlua/branch/v0.8/graph/badge.svg?token=99339FS1CG
 [codecov.io]: https://codecov.io/gh/khvzak/mlua
 [MSRV]: https://img.shields.io/badge/rust-1.56+-brightgreen.svg?&logo=rust
 

src/ffi/luau/lua.rs

@@ -7,12 +7,15 @@ use std::ptr;
 // Option for multiple returns in 'lua_pcall' and 'lua_call'
 pub const LUA_MULTRET: c_int = -1;
 
+// Max number of Lua stack slots
+const LUAI_MAXCSTACK: c_int = 100000;
+
 //
 // Pseudo-indices
 //
-pub const LUA_REGISTRYINDEX: c_int = -10000;
-pub const LUA_ENVIRONINDEX: c_int = -10001;
-pub const LUA_GLOBALSINDEX: c_int = -10002;
+pub const LUA_REGISTRYINDEX: c_int = -LUAI_MAXCSTACK - 2000;
+pub const LUA_ENVIRONINDEX: c_int = -LUAI_MAXCSTACK - 2001;
+pub const LUA_GLOBALSINDEX: c_int = -LUAI_MAXCSTACK - 2002;
 
 pub const fn lua_upvalueindex(i: c_int) -> c_int {
     LUA_GLOBALSINDEX - i

src/hook.rs

@@ -63,7 +63,7 @@ impl<'lua> Debug<'lua> {
     }
 
     /// Corresponds to the `n` what mask.
-    pub fn names(&self) -> DebugNames<'lua> {
+    pub fn names(&self) -> DebugNames {
         unsafe {
             #[cfg(not(feature = "luau"))]
             mlua_assert!(
@@ -87,7 +87,7 @@ impl<'lua> Debug<'lua> {
     }
 
     /// Corresponds to the `S` what mask.
-    pub fn source(&self) -> DebugSource<'lua> {
+    pub fn source(&self) -> DebugSource {
         unsafe {
             #[cfg(not(feature = "luau"))]
             mlua_assert!(

src/lib.rs

@@ -72,7 +72,7 @@
 //! [`serde::Deserialize`]: https://docs.serde.rs/serde/de/trait.Deserialize.html
 
 // mlua types in rustdoc of other crates get linked to here.
-#![doc(html_root_url = "https://docs.rs/mlua/0.8.5")]
+#![doc(html_root_url = "https://docs.rs/mlua/0.8.8")]
 // Deny warnings inside doc tests / examples. When this isn't present, rustdoc doesn't show *any*
 // warnings at all.
 #![doc(test(attr(deny(warnings))))]

src/lua.rs

@@ -88,6 +88,8 @@ pub(crate) struct ExtraData {
 
     registered_userdata: FxHashMap<TypeId, c_int>,
     registered_userdata_mt: FxHashMap<*const c_void, Option<TypeId>>,
+
+    // When Lua instance dropped, setting `None` would prevent collecting `RegistryKey`s
     registry_unref_list: Arc<Mutex<Option<Vec<c_int>>>>,
 
     #[cfg(not(feature = "send"))]
@@ -2060,34 +2062,38 @@ impl Lua {
     /// [`RegistryKey`]: crate::RegistryKey
     pub fn create_registry_value<'lua, T: ToLua<'lua>>(&'lua self, t: T) -> Result<RegistryKey> {
         let t = t.to_lua(self)?;
+        if t == Value::Nil {
+            // Special case to skip calling `luaL_ref` and use `LUA_REFNIL` instead
+            let unref_list = unsafe { (*self.extra.get()).registry_unref_list.clone() };
+            return Ok(RegistryKey::new(ffi::LUA_REFNIL, unref_list));
+        }
+
         unsafe {
             let _sg = StackGuard::new(self.state);
             check_stack(self.state, 4)?;
 
-            let unref_list = (*self.extra.get()).registry_unref_list.clone();
             self.push_value(t)?;
 
-            // Try to reuse previously allocated RegistryKey
-            let unref_list2 = unref_list.clone();
-            let mut unref_list2 = mlua_expect!(unref_list2.lock(), "unref list poisoned");
-            if let Some(registry_id) = unref_list2.as_mut().and_then(|x| x.pop()) {
+            // Try to reuse previously allocated slot
+            let unref_list = (*self.extra.get()).registry_unref_list.clone();
+            let free_registry_id = mlua_expect!(unref_list.lock(), "unref list poisoned")
+                .as_mut()
+                .and_then(|x| x.pop());
+            if let Some(registry_id) = free_registry_id {
                 // It must be safe to replace the value without triggering memory error
                 ffi::lua_rawseti(self.state, ffi::LUA_REGISTRYINDEX, registry_id as Integer);
-                return Ok(RegistryKey {
-                    registry_id,
-                    unref_list,
-                });
+                return Ok(RegistryKey::new(registry_id, unref_list));
             }
 
             // Allocate a new RegistryKey
-            let registry_id = protect_lua!(self.state, 1, 0, |state| {
-                ffi::luaL_ref(state, ffi::LUA_REGISTRYINDEX)
-            })?;
-
-            Ok(RegistryKey {
-                registry_id,
-                unref_list,
-            })
+            let registry_id = if self.unlikely_memory_error() {
+                ffi::luaL_ref(self.state, ffi::LUA_REGISTRYINDEX)
+            } else {
+                protect_lua!(self.state, 1, 0, |state| {
+                    ffi::luaL_ref(state, ffi::LUA_REGISTRYINDEX)
+                })?
+            };
+            Ok(RegistryKey::new(registry_id, unref_list))
         }
     }
 
@@ -2102,13 +2108,16 @@ impl Lua {
             return Err(Error::MismatchedRegistryKey);
         }
 
-        let value = unsafe {
-            let _sg = StackGuard::new(self.state);
-            check_stack(self.state, 1)?;
+        let value = match key.is_nil() {
+            true => Value::Nil,
+            false => unsafe {
+                let _sg = StackGuard::new(self.state);
+                check_stack(self.state, 1)?;
 
-            let id = key.registry_id as Integer;
-            ffi::lua_rawgeti(self.state, ffi::LUA_REGISTRYINDEX, id);
-            self.pop_value()
+                let id = key.registry_id as Integer;
+                ffi::lua_rawgeti(self.state, ffi::LUA_REGISTRYINDEX, id);
+                self.pop_value()
+            },
         };
         T::from_lua(value, self)
     }
@@ -2147,13 +2156,28 @@ impl Lua {
         }
 
         let t = t.to_lua(self)?;
+        if t == Value::Nil && key.is_nil() {
+            // Nothing to replace
+            return Ok(());
+        } else if t != Value::Nil && key.registry_id == ffi::LUA_REFNIL {
+            // We cannot update `LUA_REFNIL` slot
+            let err = "cannot replace nil value with non-nil".to_string();
+            return Err(Error::RuntimeError(err));
+        }
+
         unsafe {
             let _sg = StackGuard::new(self.state);
             check_stack(self.state, 2)?;
 
-            self.push_value(t)?;
-            // It must be safe to replace the value without triggering memory error
             let id = key.registry_id as Integer;
+            if t == Value::Nil {
+                self.push_value(Value::Integer(id))?;
+                key.set_nil(true);
+            } else {
+                self.push_value(t)?;
+                key.set_nil(false);
+            }
+            // It must be safe to replace the value without triggering memory error
             ffi::lua_rawseti(self.state, ffi::LUA_REGISTRYINDEX, id);
         }
         Ok(())

src/luau.rs

@@ -22,6 +22,12 @@ impl Lua {
         globals.raw_set("require", self.create_function(lua_require)?)?;
         globals.raw_set("vector", self.create_c_function(lua_vector)?)?;
 
+        // Set `_VERSION` global to include version number
+        // The environment variable `LUAU_VERSION` set by the build script
+        if let Some(version) = option_env!("LUAU_VERSION") {
+            globals.raw_set("_VERSION", format!("Luau {version}"))?;
+        }
+
         Ok(())
     }
 }

src/types.rs

@@ -1,6 +1,7 @@
 use std::cell::UnsafeCell;
 use std::hash::{Hash, Hasher};
 use std::os::raw::{c_int, c_void};
+use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{Arc, Mutex};
 use std::{fmt, mem, ptr};
 
@@ -104,6 +105,7 @@ pub(crate) struct DestructedUserdata;
 /// [`AnyUserData::get_user_value`]: crate::AnyUserData::get_user_value
 pub struct RegistryKey {
     pub(crate) registry_id: c_int,
+    pub(crate) is_nil: AtomicBool,
     pub(crate) unref_list: Arc<Mutex<Option<Vec<c_int>>>>,
 }
 
@@ -129,15 +131,27 @@ impl Eq for RegistryKey {}
 
 impl Drop for RegistryKey {
     fn drop(&mut self) {
-        let mut unref_list = mlua_expect!(self.unref_list.lock(), "unref list poisoned");
-        if let Some(list) = unref_list.as_mut() {
-            list.push(self.registry_id);
+        // We don't need to collect nil slot
+        if self.registry_id > ffi::LUA_REFNIL {
+            let mut unref_list = mlua_expect!(self.unref_list.lock(), "unref list poisoned");
+            if let Some(list) = unref_list.as_mut() {
+                list.push(self.registry_id);
+            }
         }
     }
 }
 
 impl RegistryKey {
-    // Destroys the RegistryKey without adding to the drop list
+    // Creates a new instance of `RegistryKey`
+    pub(crate) const fn new(id: c_int, unref_list: Arc<Mutex<Option<Vec<c_int>>>>) -> Self {
+        RegistryKey {
+            registry_id: id,
+            is_nil: AtomicBool::new(id == ffi::LUA_REFNIL),
+            unref_list,
+        }
+    }
+
+    // Destroys the `RegistryKey` without adding to the unref list
     pub(crate) fn take(self) -> c_int {
         let registry_id = self.registry_id;
         unsafe {
@@ -146,6 +160,21 @@ impl RegistryKey {
         }
         registry_id
     }
+
+    // Returns true if this `RegistryKey` holds a nil value
+    #[inline(always)]
+    pub(crate) fn is_nil(&self) -> bool {
+        self.is_nil.load(Ordering::Relaxed)
+    }
+
+    // Marks value of this `RegistryKey` as `Nil`
+    #[inline(always)]
+    pub(crate) fn set_nil(&self, enabled: bool) {
+        // We cannot replace previous value with nil in as this will break
+        // Lua mechanism to find free keys.
+        // Instead, we set a special flag to mark value as nil.
+        self.is_nil.store(enabled, Ordering::Relaxed);
+    }
 }
 
 pub(crate) struct LuaRef<'lua> {

src/util.rs

@@ -245,7 +245,8 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error {
 // Uses 3 (or 1 if unprotected) stack spaces, does not call checkstack.
 #[inline(always)]
 pub unsafe fn push_string(state: *mut ffi::lua_State, s: &[u8], protect: bool) -> Result<()> {
-    if protect {
+    // Always use protected mode if the string is too long
+    if protect || s.len() > (1 << 30) {
         protect_lua!(state, 0, 1, |state| {
             ffi::lua_pushlstring(state, s.as_ptr() as *const c_char, s.len());
         })
@@ -427,11 +428,17 @@ unsafe fn init_userdata_metatable_index(state: *mut ffi::lua_State) -> Result<()
     }
     ffi::lua_pop(state, 1);
 
-    // Create and cache `__index` helper
+    // Create and cache `__index` generator
     let code = cstr!(
         r#"
             local error, isfunction = ...
             return function (__index, field_getters, methods)
+                -- Fastpath to return methods table for index access
+                if __index == nil and field_getters == nil then
+                    return methods
+                end
+
+                -- Alternatively return a function for index access
                 return function (self, key)
                     if field_getters ~= nil then
                         local field_getter = field_getters[key]
@@ -481,7 +488,7 @@ pub unsafe fn init_userdata_metatable_newindex(state: *mut ffi::lua_State) -> Re
     }
     ffi::lua_pop(state, 1);
 
-    // Create and cache `__newindex` helper
+    // Create and cache `__newindex` generator
     let code = cstr!(
         r#"
             local error, isfunction = ...

src/value.rs

@@ -197,7 +197,6 @@ pub struct MultiValue<'lua>(Vec<Value<'lua>>);
 
 impl<'lua> MultiValue<'lua> {
     /// Creates an empty `MultiValue` containing no values.
-    #[inline]
     pub const fn new() -> MultiValue<'lua> {
         MultiValue(Vec::new())
     }
@@ -276,7 +275,10 @@ impl<'lua> MultiValue<'lua> {
 
     #[inline]
     pub fn get(&self, index: usize) -> Option<&Value<'lua>> {
-        self.0.get(self.0.len() - index - 1)
+        if index < self.0.len() {
+            return self.0.get(self.0.len() - index - 1);
+        }
+        None
     }
 
     #[inline]

tests/tests.rs

@@ -276,7 +276,10 @@ fn test_error() -> Result<()> {
             end, 3)
 
             local function handler(err)
-                if string.match(_VERSION, ' 5%.1$') or string.match(_VERSION, ' 5%.2$') or _VERSION == "Luau" then
+                if string.match(_VERSION, " 5%.1$")
+                    or string.match(_VERSION, " 5%.2$")
+                    or string.match(_VERSION, "Luau")
+                then
                     -- Special case for Lua 5.1/5.2 and Luau
                     local caps = string.match(err, ': (%d+)$')
                     if caps then
@@ -793,6 +796,17 @@ fn test_replace_registry_value() -> Result<()> {
     let key = lua.create_registry_value::<i32>(42)?;
     lua.replace_registry_value(&key, "new value")?;
     assert_eq!(lua.registry_value::<String>(&key)?, "new value");
+    lua.replace_registry_value(&key, Value::Nil)?;
+    assert_eq!(lua.registry_value::<Value>(&key)?, Value::Nil);
+    lua.replace_registry_value(&key, 123)?;
+    assert_eq!(lua.registry_value::<i32>(&key)?, 123);
+
+    // It should be impossible to replace (initial) nil value with non-nil
+    let key2 = lua.create_registry_value(Value::Nil)?;
+    match lua.replace_registry_value(&key2, "abc") {
+        Err(Error::RuntimeError(_)) => {}
+        r => panic!("expected RuntimeError, got {r:?}"),
+    }
 
     Ok(())
 }
@@ -844,6 +858,28 @@ fn test_mismatched_registry_key() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn test_registry_value_reuse() -> Result<()> {
+    let lua = Lua::new();
+
+    let r1 = lua.create_registry_value("value1")?;
+    let r1_slot = format!("{r1:?}");
+    drop(r1);
+
+    // Previous slot must not be reused by nil value
+    let r2 = lua.create_registry_value(Value::Nil)?;
+    let r2_slot = format!("{r2:?}");
+    assert_ne!(r1_slot, r2_slot);
+    drop(r2);
+
+    // But should be reused by non-nil value
+    let r3 = lua.create_registry_value("value3")?;
+    let r3_slot = format!("{r3:?}");
+    assert_eq!(r1_slot, r3_slot);
+
+    Ok(())
+}
+
 #[test]
 fn test_application_data() -> Result<()> {
     let lua = Lua::new();

tests/value.rs

@@ -1,6 +1,6 @@
 use std::ptr;
 
-use mlua::{Lua, Result, Value};
+use mlua::{Lua, MultiValue, Result, Value};
 
 #[test]
 fn test_value_eq() -> Result<()> {
@@ -63,3 +63,21 @@ fn test_value_eq() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_multi_value() {
+    let mut multi_value = MultiValue::new();
+    assert_eq!(multi_value.len(), 0);
+    assert_eq!(multi_value.get(0), None);
+
+    multi_value.push_front(Value::Number(2.));
+    multi_value.push_front(Value::Number(1.));
+    assert_eq!(multi_value.get(0), Some(&Value::Number(1.)));
+    assert_eq!(multi_value.get(1), Some(&Value::Number(2.)));
+
+    assert_eq!(multi_value.pop_front(), Some(Value::Number(1.)));
+    assert_eq!(multi_value[0], Value::Number(2.));
+
+    multi_value.clear();
+    assert!(multi_value.is_empty());
+}