From cf0e90d7c5fe23c49e43c828130c5849daaa98d0 Mon Sep 17 00:00:00 2001 From: hadashiA Date: Mon, 13 Jul 2026 09:06:49 +0900 Subject: [PATCH 1/3] Fix symbol aliasing on FNV-1a hash collision --- .../ChibiRubySourceGenerator.cs | 23 ++---- src/ChibiRuby/Symbol.cs | 73 ++++++++++++------- 2 files changed, 53 insertions(+), 43 deletions(-) diff --git a/src/ChibiRuby.SourceGenerator/ChibiRubySourceGenerator.cs b/src/ChibiRuby.SourceGenerator/ChibiRubySourceGenerator.cs index 750bdf9b..a8e22134 100644 --- a/src/ChibiRuby.SourceGenerator/ChibiRubySourceGenerator.cs +++ b/src/ChibiRuby.SourceGenerator/ChibiRubySourceGenerator.cs @@ -227,32 +227,23 @@ public static bool TryFind(int hashCode, ReadOnlySpan name, out Symbol sym stringBuilder.AppendLine($$""" case {{x.Key}}: """); - if (x.Count() == 1) + // Always verify the actual name: a user-defined symbol may collide with a + // known symbol's 32-bit hash, and must not alias it. + var branch = "if"; + foreach (var xs in x) { - var singleValue = x.First(); stringBuilder.AppendLine($$""" - symbol = new Symbol({{singleValue.Index}}); - return true; -"""); - } - else - { - var branch = "if"; - foreach (var xs in x) - { - stringBuilder.AppendLine($$""" {{branch}} (name.SequenceEqual("{{xs.SymbolName}}"u8)) { symbol = new Symbol({{xs.Index}}); return true; } """); - branch = "else if"; - } - stringBuilder.AppendLine($$""" + branch = "else if"; + } + stringBuilder.AppendLine($$""" break; """); - } } stringBuilder.AppendLine($$""" } diff --git a/src/ChibiRuby/Symbol.cs b/src/ChibiRuby/Symbol.cs index 5517b135..3d3b60d7 100644 --- a/src/ChibiRuby/Symbol.cs +++ b/src/ChibiRuby/Symbol.cs @@ -13,23 +13,30 @@ public readonly record struct Symbol(uint Value) class SymbolTable { - readonly record struct Key(int HashCode) + /// + /// One interned symbol per node, chained per FNV-1a hash bucket. Names sharing a + /// hash coexist on the chain and are distinguished by comparing the actual bytes, + /// so a 32-bit hash collision can never alias two different symbol names. + /// + sealed class Entry(Symbol symbol, byte[] name, Entry? next) { - const uint OffsetBasis = 2166136261u; - const uint FnvPrime = 16777619u; + public readonly Symbol Symbol = symbol; + public readonly byte[] Name = name; + public readonly Entry? Next = next; + } + + const uint OffsetBasis = 2166136261u; + const uint FnvPrime = 16777619u; - public static Key Create(ReadOnlySpan symbolName) + static int HashOf(ReadOnlySpan symbolName) + { + var hash = OffsetBasis; + foreach (var b in symbolName) { - var hash = OffsetBasis; - foreach (var b in symbolName) - { - hash ^= b; - hash *= FnvPrime; - } - return new Key(unchecked((int)hash)); + hash ^= b; + hash *= FnvPrime; } - - public override int GetHashCode() => HashCode; + return unchecked((int)hash); } const int PackLengthMax = 5; @@ -44,7 +51,7 @@ public static Key Create(ReadOnlySpan symbolName) static byte[] ThreadStaticBuffer() => nameBuffer ??= new byte[32]; readonly Dictionary names = new(64); - readonly Dictionary symbols = new(64); + readonly Dictionary symbols = new(64); public Symbol Intern(ReadOnlySpan utf8) { @@ -52,13 +59,7 @@ public Symbol Intern(ReadOnlySpan utf8) { return symbol; } - - symbol = new Symbol(++lastId); - var nameBuf = new byte[utf8.Length]; - utf8.CopyTo(nameBuf); - names.Add(symbol, nameBuf); - symbols.Add(Key.Create(utf8), symbol); - return symbol; + return Add(utf8.ToArray()); } public Symbol InternLiteral(byte[] utf8) @@ -67,9 +68,16 @@ public Symbol InternLiteral(byte[] utf8) { return symbol; } - symbol = new Symbol(++lastId); - names.Add(symbol, utf8); - symbols.Add(Key.Create(utf8), symbol); + return Add(utf8); + } + + Symbol Add(byte[] name) + { + var symbol = new Symbol(++lastId); + names.Add(symbol, name); + var hash = HashOf(name); + symbols.TryGetValue(hash, out var head); + symbols[hash] = new Entry(symbol, name, head); return symbol; } @@ -92,9 +100,20 @@ public bool TryFind(ReadOnlySpan utf8, out Symbol symbol) // { // return true; // } - var key = Key.Create(utf8); - return symbols.TryGetValue(key, out symbol) || - Names.TryFind(key.HashCode, utf8, out symbol); + var hash = HashOf(utf8); + if (symbols.TryGetValue(hash, out var entry)) + { + do + { + if (entry.Name.AsSpan().SequenceEqual(utf8)) + { + symbol = entry.Symbol; + return true; + } + entry = entry.Next; + } while (entry != null); + } + return Names.TryFind(hash, utf8, out symbol); } [MethodImpl(MethodImplOptions.AggressiveInlining)] From 00ca831c6c5e0a17093b9c4718973ca522c99d40 Mon Sep 17 00:00:00 2001 From: hadashiA Date: Mon, 13 Jul 2026 09:06:49 +0900 Subject: [PATCH 2/3] Memoize OP_SYMBOL pool symbols per irep --- src/ChibiRuby/Irep.cs | 13 +++++++++++++ src/ChibiRuby/MRubyState.Init.cs | 9 +++++++++ src/ChibiRuby/MRubyState.Vm.cs | 30 ++++++++++++++++++++++++++++-- 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/ChibiRuby/Irep.cs b/src/ChibiRuby/Irep.cs index ad36a28f..c03062a9 100644 --- a/src/ChibiRuby/Irep.cs +++ b/src/ChibiRuby/Irep.cs @@ -29,11 +29,24 @@ public readonly struct CatchHandler(CatchHandlerType handlerType, uint begin, ui public readonly uint Target = target; } +/// +/// Memoized results of interning this irep's pool strings (OP_SYMBOL operands). +/// Owner id and the symbol array live in one immutable-shape object so that a +/// state can never observe another state's symbols through a torn owner/array pair. +/// +sealed class IrepPoolSymbolCache(int ownerStateId, Symbol[] symbols) +{ + public readonly int OwnerStateId = ownerStateId; + public readonly Symbol[] Symbols = symbols; +} + /// /// Program data /// public class Irep { + internal IrepPoolSymbolCache? PoolSymbolCache; + public byte Flags { get; init; } public ushort RegisterVariableCount { get; init; } public byte[] Sequence { get; init; } = []; diff --git a/src/ChibiRuby/MRubyState.Init.cs b/src/ChibiRuby/MRubyState.Init.cs index 83561f7a..73177d20 100644 --- a/src/ChibiRuby/MRubyState.Init.cs +++ b/src/ChibiRuby/MRubyState.Init.cs @@ -80,6 +80,15 @@ public static MRubyState Create() public RiteParser RiteParser => riteParser ??= new RiteParser(this); + static int stateIdSource; + + /// + /// Process-wide unique id of this state. Symbols are interned per state, so + /// caches of interned symbols (e.g. ) are + /// tagged with this id to stay valid when an Irep is shared between states. + /// + internal readonly int StateId = System.Threading.Interlocked.Increment(ref stateIdSource); + readonly SymbolTable symbolTable = new(); readonly VariableTable globalVariables = new(); diff --git a/src/ChibiRuby/MRubyState.Vm.cs b/src/ChibiRuby/MRubyState.Vm.cs index 6ef48577..55502453 100644 --- a/src/ChibiRuby/MRubyState.Vm.cs +++ b/src/ChibiRuby/MRubyState.Vm.cs @@ -492,6 +492,25 @@ internal MRubyValue EvalUnder(MRubyValue self, RProc block, RClass c) /// /// Execute irep assuming the Stack values are placed /// + /// + /// OP_SYMBOL slow path: intern the pool string and memoize it on the irep. + /// A cache owned by another state is replaced wholesale (owner id and symbol + /// array travel together), so shared ireps stay correct across states. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + static Symbol PoolSymbolMiss(MRubyState state, Irep irep, int poolIndex) + { + var symbol = state.Intern(irep.PoolValues[poolIndex].As()); + var cache = irep.PoolSymbolCache; + if (cache == null || cache.OwnerStateId != state.StateId) + { + cache = new IrepPoolSymbolCache(state.StateId, new Symbol[irep.PoolValues.Length]); + irep.PoolSymbolCache = cache; + } + cache.Symbols[poolIndex] = symbol; + return symbol; + } + internal MRubyValue Execute(Irep irep, int pc, int stackKeep, RException? injectedRaise = null) { Exception = null; @@ -2444,8 +2463,15 @@ static void APostLong(MRubyState state, RArray array, int pre, int post, ref MRu { Markers.Symbol(); bb = OperandBB.Read(ref sequence, ref callInfo.ProgramCounter); - //var name = irep.PoolValues[bb.B].As(); - Unsafe.Add(ref registers, bb.A) = Intern(irep.PoolValues[bb.B].As()); + Symbol sym = default; + var poolSymbols = irep.PoolSymbolCache; + if (poolSymbols == null || + poolSymbols.OwnerStateId != StateId || + (sym = poolSymbols.Symbols[bb.B]).Value == 0) + { + sym = PoolSymbolMiss(this, irep, bb.B); + } + Unsafe.Add(ref registers, bb.A) = sym; goto Next; } case OpCode.String: From d78435f4550f1f6915cf8081fb5979619723ab60 Mon Sep 17 00:00:00 2001 From: hadashiA Date: Mon, 13 Jul 2026 09:06:49 +0900 Subject: [PATCH 3/3] Add symbol collision and OP_SYMBOL memo tests --- tests/ChibiRuby.Tests/SymbolTest.cs | 60 +++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/ChibiRuby.Tests/SymbolTest.cs b/tests/ChibiRuby.Tests/SymbolTest.cs index dafc4b82..9b2358e8 100644 --- a/tests/ChibiRuby.Tests/SymbolTest.cs +++ b/tests/ChibiRuby.Tests/SymbolTest.cs @@ -14,4 +14,64 @@ public void InlinePack() Assert.That(name.SequenceEqual("call"u8), Is.True); // Assert.That(symbolTable.Intern("call"u8), Is.EqualTo(Names.Call)); } + + [Test] + public void InternFnv32CollidingNames() + { + // "costarring" and "liquid" are a known FNV-1a 32-bit collision pair; + // they must intern to distinct symbols with round-tripping names. + var symbolTable = new SymbolTable(); + + var costarring = symbolTable.Intern("costarring"u8); + var liquid = symbolTable.Intern("liquid"u8); + + Assert.That(costarring, Is.Not.EqualTo(liquid)); + Assert.That(symbolTable.NameOf(costarring).SequenceEqual("costarring"u8), Is.True); + Assert.That(symbolTable.NameOf(liquid).SequenceEqual("liquid"u8), Is.True); + + // both remain findable after the collision chain forms + Assert.That(symbolTable.Intern("costarring"u8), Is.EqualTo(costarring)); + Assert.That(symbolTable.Intern("liquid"u8), Is.EqualTo(liquid)); + + // another documented pair, interned in reverse order + var zinke = symbolTable.Intern("zinke"u8); + var altarage = symbolTable.Intern("altarage"u8); + Assert.That(altarage, Is.Not.EqualTo(zinke)); + Assert.That(symbolTable.NameOf(altarage).SequenceEqual("altarage"u8), Is.True); + } + + [Test] + public void PoolSymbolMemoAcrossStates() + { + // OP_SYMBOL (emitted by other mruby toolchains; the bundled compiler + // prefers OP_LOADSYM) interns a pool string and memoizes the result on + // the Irep. Symbols are per-state, so a second state executing the same + // Irep instance must not observe the first state's memo. + using var state1 = MRubyState.Create(); + using var state2 = MRubyState.Create(); + + // hand-assembled: SYMBOL R1, Pool[0]; RETURN R1; STOP + var irep = new Irep + { + RegisterVariableCount = 2, + Sequence = + [ + (byte)OpCode.Symbol, 1, 0, + (byte)OpCode.Return, 1, + (byte)OpCode.Stop, + ], + PoolValues = [new MRubyValue(state1.NewString("pool_symbol_memo_test"u8))], + }; + + var r1a = state1.Execute(irep); + var r1b = state1.Execute(irep); // memoized in state1 + var r2a = state2.Execute(irep); // state2 must re-intern, not reuse state1's memo + var r2b = state2.Execute(irep); // memoized in state2 + + Assert.That(r1a.IsSymbol, Is.True); + Assert.That(r1b, Is.EqualTo(r1a)); + Assert.That(r2b, Is.EqualTo(r2a)); + Assert.That(state1.NameOf(r1a.SymbolValue).ToString(), Is.EqualTo("pool_symbol_memo_test")); + Assert.That(state2.NameOf(r2a.SymbolValue).ToString(), Is.EqualTo("pool_symbol_memo_test")); + } }