From c6b19c79266d0465d8bb4687214aa6db41a87e27 Mon Sep 17 00:00:00 2001 From: grom Date: Mon, 2 Nov 2026 08:03:09 -0700 Subject: [PATCH] lua: a size this library derived is a cache, and a `#` append invalidates it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is what froze his client. The bound named `Cinematic/Camera.lua:2597`: while turnStarts[1] and now - turnStarts[1] > WINDOW do table.remove(turnStarts, 1) end `remove` drains the table and remembers a size of 0 — faithfully, that is `luaL_setn`. The addon then appends with `turnStarts[#turnStarts + 1] = now`, which is a raw set this layer never sees, so slot 1 is full while the remembered size is still 0. `remove` reads 0, takes its "nothing to remove" path, changes nothing, and the caller spins forever. 15 s of Lua in every frame, for the whole session, in a loop that in the reference cannot fail. It cannot fail there because 5.0 has no `#` operator: every append goes through `insert` or `getn`, so a remembered size is always the truth. The collision is a later-era addon's `#` meeting 5.0's bookkeeping, and it also loses data the quiet way round — `insert` after a `#` append writes *over* the appended slot instead of after it, which is a test here now too. So the two kinds of size are kept apart. One this library **derived** from an `insert` or a `remove` is a cache: if `t[n + 1]` is not nil the table has grown since, and the count wins. One an addon **declared** with `table.setn` is honoured exactly as before, because `setn(t, 0)` as a logical wipe over full slots is the entire point of it — AceLibrary and Mik's capture tables both turn on that, and both still pass. An explicit numeric `t.n` field is a declaration too, and is untouched. Measured: the corpus roster is byte-identical (87 ok / 1 fail, the failure the pre-existing AdvancedTradeSkillWindow2), and the drain loop that spun 44 times in his log now ends in one removal a round. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../benilla-ui/src/script/lua50/table_size.rs | 212 ++++++++++++++---- 1 file changed, 171 insertions(+), 41 deletions(-) diff --git a/crates/benilla-ui/src/script/lua50/table_size.rs b/crates/benilla-ui/src/script/lua50/table_size.rs index d666720b..6772a558 100644 --- a/crates/benilla-ui/src/script/lua50/table_size.rs +++ b/crates/benilla-ui/src/script/lua50/table_size.rs @@ -9,12 +9,90 @@ //! //! The side store must stay invisible: a stray `n` key breaks mixin walks such as `AceOO-2.0`'s, //! which error on any field the target already has. +//! +//! ## The one place 5.0's remembered size and 5.1's `#` collide +//! +//! 5.0 has no `#` operator, so in the reference nothing can grow a table behind the library's +//! back and a remembered size is always the truth. A later-era addon appends with `t[#t + 1] = v`, +//! which is a raw set this layer never sees — so after `remove` has stored a size of 0, the next +//! `#` append writes slot 1 and `remove` reads 0, refuses, and the caller spins: +//! +//! ```lua +//! while turnStarts[1] and now - turnStarts[1] > WINDOW do table.remove(turnStarts, 1) end +//! ``` +//! +//! That is `Cinematic/Camera.lua:2597`, and it froze the client twice on 2026-11-02 — 15 s of Lua +//! in every frame, for the whole session. The same mismatch silently *loses* data the other way: +//! `insert` after a `#` append writes over the appended slot instead of after it. +//! +//! So a size this library **derived** is treated as a cache: if `t[n + 1]` is not nil the table has +//! grown since, and the count wins. A size an addon **declared** with `table.setn` is still +//! honoured exactly as 5.0 honours it — that is the whole point of `setn(t, 0)` as a logical wipe +//! (`AceLibrary`, Mik's capture tables), and it is also why the two cases cannot share one slot. +//! An explicit numeric `t.n` field is a declaration too, and stays untouched. use mlua::{Function, Lua, MultiValue, Table, Value}; /// The side store's registry name; 5.0 uses slot 2, and either way no addon can reach it. const SIZES: &str = "benilla.lua50.sizes"; +/// Beside it, which of those sizes an addon **declared** with `table.setn` rather than this +/// library deriving it from an `insert` or a `remove`. Only a derived one is a cache; see the +/// module doc. +const DECLARED: &str = "benilla.lua50.sizes.declared"; + +/// The remembered size, and whether the addon declared it. Cloned into each closure below, as the +/// one table was before: two weak-keyed tables, neither reachable from Lua. +#[derive(Clone)] +struct Sizes { + store: Table, + declared: Table, +} + +impl Sizes { + /// `luaL_getn` (`0x6f5050`); its count to the first nil is not the `#` border on a holed + /// table. A size this library derived is dropped when the table has grown past it. + fn get(&self, t: &Table) -> mlua::Result { + if let Some(n) = as_size(&t.raw_get::("n")?) { + return Ok(n); + } + if let Some(n) = as_size(&self.store.raw_get::(t.clone())?) { + let declared = self.declared.raw_get::(t.clone())?; + let grown = !matches!(t.raw_get::(n + 1)?, Value::Nil); + if matches!(declared, Value::Boolean(true)) || !grown { + return Ok(n); + } + } + let mut i = 1i64; + while !matches!(t.raw_get::(i)?, Value::Nil) { + i += 1; + } + Ok(i - 1) + } + + /// `luaL_setn` (`0x6f4ea0`): into `t.n` when the table has one, else the side store. `insert` + /// and `remove` call this, so the size they write is derived and the declaration is spent. + fn set(&self, t: &Table, n: i64) -> mlua::Result<()> { + self.declared.raw_set(t.clone(), Value::Nil)?; + self.write(t, n) + } + + /// The same write from `table.setn`: the addon is stating the size, so it is honoured even + /// where the table says otherwise. + fn declare(&self, t: &Table, n: i64) -> mlua::Result<()> { + self.declared.raw_set(t.clone(), true)?; + self.write(t, n) + } + + fn write(&self, t: &Table, n: i64) -> mlua::Result<()> { + if as_size(&t.raw_get::("n")?).is_some() { + t.raw_set("n", n) + } else { + self.store.raw_set(t.clone(), n) + } + } +} + /// `checkint` (`0x6f4f80`): a stored size counts only if numeric and non-negative. The reference /// also takes a numeric string through `lua_tonumber`; this does not. fn as_size(v: &Value) -> Option { @@ -34,55 +112,38 @@ fn as_size(v: &Value) -> Option { /// is beyond the VM's instruction budget: a hung client. const MAX_SHIFT: i64 = 1_000_000; -/// `luaL_getn` (`0x6f5050`); its count to the first nil is not the `#` border on a holed table. -fn get_n(sizes: &Table, t: &Table) -> mlua::Result { - if let Some(n) = as_size(&t.raw_get::("n")?) { - return Ok(n); - } - if let Some(n) = as_size(&sizes.raw_get::(t.clone())?) { - return Ok(n); - } - let mut i = 1i64; - while !matches!(t.raw_get::(i)?, Value::Nil) { - i += 1; - } - Ok(i - 1) -} - -/// `luaL_setn` (`0x6f4ea0`): into `t.n` when the table has one, else the side store. -fn set_n(sizes: &Table, t: &Table, n: i64) -> mlua::Result<()> { - if as_size(&t.raw_get::("n")?).is_some() { - t.raw_set("n", n) - } else { - sizes.raw_set(t.clone(), n) - } -} - /// Install the `n`-based table library over mlua's 5.1 one. `concat` and `sort` wrap the C bodies /// for their error messages and algorithm (Rust's `sort_by` panics on an inconsistent comparator). pub(super) fn install(lua: &Lua) -> mlua::Result<()> { let g = lua.globals(); - let store = lua.create_table()?; - let meta = lua.create_table()?; // Weak keys, as 5.0's `getsizes` builds it: a table nobody else holds stays collectable. - meta.set("__mode", "k")?; - store.set_metatable(Some(meta))?; - // Held by the registry for the VM's life; each closure below captures it. - lua.set_named_registry_value(SIZES, store.clone())?; - let sizes = store; + let weak = |lua: &Lua| -> mlua::Result { + let t = lua.create_table()?; + let meta = lua.create_table()?; + meta.set("__mode", "k")?; + t.set_metatable(Some(meta))?; + Ok(t) + }; + let sizes = Sizes { + store: weak(lua)?, + declared: weak(lua)?, + }; + // Held by the registry for the VM's life; each closure below captures its own clone. + lua.set_named_registry_value(SIZES, sizes.store.clone())?; + lua.set_named_registry_value(DECLARED, sizes.declared.clone())?; let t: Table = g.get("table")?; let getn_sizes = sizes.clone(); t.set( "getn", - lua.create_function(move |_, t: Table| get_n(&getn_sizes, &t))?, + lua.create_function(move |_, t: Table| getn_sizes.get(&t))?, )?; // `table.setn` (`0x7fb670`): no results. let setn_sizes = sizes.clone(); t.set( "setn", - lua.create_function(move |_, (t, n): (Table, i64)| set_n(&setn_sizes, &t, n))?, + lua.create_function(move |_, (t, n): (Table, i64)| setn_sizes.declare(&t, n))?, )?; // `table.insert` (`0x7fb6a0`): `n = getn(t) + 1`, raised to a later position; `setn` first, @@ -98,7 +159,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { "bad argument #1 to 'insert' (table expected)", )); }; - let mut n = get_n(&insert_sizes, &t)? + 1; + let mut n = insert_sizes.get(&t)? + 1; // The 2-vs-3 argument split is `lua_gettop`, so an explicit trailing nil counts: // `insert(t, 4, nil)` writes nil at 4, it does not append the 4. let (pos, v) = if argc <= 2 { @@ -122,7 +183,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { if n.saturating_sub(pos) > MAX_SHIFT { return Err(mlua::Error::runtime("table too big to insert into")); } - set_n(&insert_sizes, &t, n)?; + insert_sizes.set(&t, n)?; while n > pos { let above: Value = t.raw_get(n - 1)?; t.raw_set(n, above)?; @@ -138,7 +199,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { t.set( "remove", lua.create_function(move |_, (t, pos): (Table, Option)| { - let n = get_n(&remove_sizes, &t)?; + let n = remove_sizes.get(&t)?; let pos = pos.unwrap_or(n); if n <= 0 { return Ok(MultiValue::new()); @@ -146,7 +207,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { if n.saturating_sub(pos) > MAX_SHIFT { return Err(mlua::Error::runtime("table too big to remove from")); } - set_n(&remove_sizes, &t, n - 1)?; + remove_sizes.set(&t, n - 1)?; let taken: Value = t.raw_get(pos)?; for i in pos..n { let next: Value = t.raw_get(i + 1)?; @@ -162,7 +223,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { t.set( "foreachi", lua.create_function(move |_, (t, f): (Table, Function)| { - let n = get_n(&foreachi_sizes, &t)?; + let n = foreachi_sizes.get(&t)?; for i in 1..=n { let v: Value = t.raw_get(i)?; let r: Value = f.call((i, v))?; @@ -183,7 +244,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { move |_, (t, sep, i, j): (Table, Option, Option, Option)| { let j = match j { Some(j) => j, - None => get_n(&concat_sizes, &t)?, + None => concat_sizes.get(&t)?, }; concat.call::((t, sep.unwrap_or_default(), i.unwrap_or(1), j)) }, @@ -198,7 +259,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { t.set( "sort", lua.create_function(move |_, (t, cmp): (Table, Option)| { - let n = get_n(&sort_sizes, &t)?; + let n = sort_sizes.get(&t)?; let border = i64::try_from(t.raw_len()).unwrap_or(i64::MAX); if n >= border { return sort.call::<()>((t, cmp)); @@ -226,7 +287,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { let i = i.unwrap_or(1); let j = match j { Some(j) => j, - None => get_n(&sizes, &t)?, + None => sizes.get(&t)?, }; // The reference's `luaL_checkstack(n, "table too big to unpack")`, at `MAX_SHIFT`. if j.saturating_sub(i) >= MAX_SHIFT { @@ -247,6 +308,75 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { mod tests { use crate::script::UiScript; + /// The shape that froze the client: `remove` drains a table to a remembered 0, a later-era + /// addon appends with `#`, and `remove` is asked again. `Cinematic/Camera.lua:2597` is this + /// loop, and it spent 15 s of every frame in it for a whole session. + #[test] + fn a_hash_append_after_a_drain_is_removable_again() { + let s = UiScript::new().unwrap(); + assert_eq!( + s.eval::( + r#" + local t, spins = {}, 0 + for round = 1, 3 do + t[#t + 1] = round + while t[1] and spins < 50 do table.remove(t, 1) spins = spins + 1 end + end + return spins + "# + ) + .unwrap(), + 3, + "one removal per round; 50 is the cap, and the cap means the drain never ends" + ); + } + + /// The same mismatch the other way, which loses data instead of hanging: `insert` must land + /// after a slot the `#` append wrote, not on top of it. + #[test] + fn insert_after_a_hash_append_does_not_overwrite_it() { + let s = UiScript::new().unwrap(); + assert_eq!( + s.eval::( + r#" + local t = {} + table.insert(t, "a") + t[#t + 1] = "b" + table.insert(t, "c") + return table.concat(t, ",") + "# + ) + .unwrap(), + "a,b,c", + "a derived size that the table has outgrown is stale, so insert must count instead" + ); + } + + /// And the declaration it must not break: `setn(t, 0)` is 5.0's logical wipe, honoured even + /// though every slot it covers is still full. Only a size this library *derived* is a cache. + #[test] + fn an_explicit_setn_still_wins_over_the_tables_own_contents() { + let s = UiScript::new().unwrap(); + assert_eq!( + s.eval::("local t = {1,2,3} table.setn(t, 0) return table.getn(t)") + .unwrap(), + 0, + "the addon declared 0 with slots 1..3 full; that is the whole point of setn" + ); + assert_eq!( + s.eval::("local t = {1,2,3} table.setn(t, 0) table.insert(t, 9) return t[1]") + .unwrap(), + 9, + "5.0's insert writes getn+1, which after setn(t, 0) is index 1" + ); + assert_eq!( + s.eval::("local t = {1,2,3} table.setn(t, 7) return table.getn(t)") + .unwrap(), + 7, + "a declaration past the end is honoured too" + ); + } + /// Mik's Scrolling Battle Text 4.43's capture tables: wipe, `setn(t, 0)`, then insert. #[test] fn a_wipe_then_setn_zero_then_insert_is_countable_again() {