From a45a469a25367e18e69e68d1d57a85872756fd7c Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Tue, 8 Sep 2026 13:46:20 -0700 Subject: [PATCH 1/2] Enforce text-only input for Lua scripts Track internally generated chunks separately so cached and embedded-host workflows retain their existing behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Lua/LuaScriptCacheOperations.cs | 4 +- libs/server/Lua/LuaCommands.cs | 6 +- libs/server/Lua/LuaRunner.Functions.cs | 19 +++-- libs/server/Lua/LuaRunner.Loader.cs | 16 ++-- libs/server/Lua/LuaRunner.cs | 20 ++++- libs/server/Lua/LuaScriptHandle.cs | 24 +++++- libs/server/Lua/LuaStateWrapper.cs | 33 +------- libs/server/Lua/NativeMethods.cs | 21 ++--- libs/server/Lua/SessionScriptCache.cs | 5 +- .../Garnet.test.scripting/LuaScriptTests.cs | 79 +++++++++++++++++++ 10 files changed, 160 insertions(+), 67 deletions(-) diff --git a/benchmark/BDN.benchmark/Lua/LuaScriptCacheOperations.cs b/benchmark/BDN.benchmark/Lua/LuaScriptCacheOperations.cs index 72558d01f1c..9bdb5707d83 100644 --- a/benchmark/BDN.benchmark/Lua/LuaScriptCacheOperations.cs +++ b/benchmark/BDN.benchmark/Lua/LuaScriptCacheOperations.cs @@ -82,7 +82,7 @@ public void IterationSetup() // Make outer hit available for every iteration LuaScriptHandle scriptHandle = null; - if (!sessionScriptCache.TryLoad(session, "return 1"u8, new(outerHitDigest), ref scriptHandle, out _, out _)) + if (!sessionScriptCache.TryLoad(session, "return 1"u8, LuaScriptChunkKind.Text, new(outerHitDigest), ref scriptHandle, out _, out _)) { throw new InvalidOperationException("Should have been able to load"); } @@ -148,7 +148,7 @@ private void LoadScript(Span digest) if (storeWrapper.storeScriptCache.TryGetValue(digestKey, out var scriptHandle)) { LuaScriptHandle newScriptHandle = null; - if (!sessionScriptCache.TryLoad(session, scriptHandle.ScriptData.Span, digestKey, ref newScriptHandle, out runner, out _)) + if (!sessionScriptCache.TryLoad(session, scriptHandle.ScriptData.Span, scriptHandle.Chunk.Kind, digestKey, ref newScriptHandle, out runner, out _)) { // TryLoad will have written an error out, it any diff --git a/libs/server/Lua/LuaCommands.cs b/libs/server/Lua/LuaCommands.cs index 4a37d88e902..9c57404e37e 100644 --- a/libs/server/Lua/LuaCommands.cs +++ b/libs/server/Lua/LuaCommands.cs @@ -48,7 +48,7 @@ private unsafe bool TryEVALSHA() { if (storeWrapper.storeScriptCache.TryGetValue(scriptKey, out var globalScriptHandle)) { - if (!sessionScriptCache.TryLoad(this, globalScriptHandle.ScriptData.Span, scriptKey, ref globalScriptHandle, out runner, out _)) + if (!sessionScriptCache.TryLoad(this, globalScriptHandle.ScriptData.Span, globalScriptHandle.Chunk.Kind, scriptKey, ref globalScriptHandle, out runner, out _)) { // TryLoad will have written an error out, it any // @@ -118,7 +118,7 @@ private unsafe bool TryEVAL() var sessionScriptHandle = globalScriptHandle; - if (!sessionScriptCache.TryLoad(this, script.ReadOnlySpan, onStackScriptKey, ref sessionScriptHandle, out var runner, out var digestOnHeap)) + if (!sessionScriptCache.TryLoad(this, script.ReadOnlySpan, LuaScriptChunkKind.Text, onStackScriptKey, ref sessionScriptHandle, out var runner, out var digestOnHeap)) { // TryLoad will have written any errors out return true; @@ -278,7 +278,7 @@ private bool NetworkScriptLoad() _ = storeWrapper.storeScriptCache.TryGetValue(onStackScriptHashKey, out var globalScriptHandle); var sessionScriptHandle = globalScriptHandle; - if (sessionScriptCache.TryLoad(this, source.ReadOnlySpan, onStackScriptHashKey, ref sessionScriptHandle, out _, out var digestOnHeap)) + if (sessionScriptCache.TryLoad(this, source.ReadOnlySpan, LuaScriptChunkKind.Text, onStackScriptHashKey, ref sessionScriptHandle, out _, out var digestOnHeap)) { // TryLoad will write any errors out diff --git a/libs/server/Lua/LuaRunner.Functions.cs b/libs/server/Lua/LuaRunner.Functions.cs index 23a6295df57..42f85ed02ec 100644 --- a/libs/server/Lua/LuaRunner.Functions.cs +++ b/libs/server/Lua/LuaRunner.Functions.cs @@ -852,7 +852,7 @@ internal int LoadString(nint luaStatePtr) return LuaWrappedError(1, constStrs.InsufficientLuaStackSpace); } - var res = state.LoadString(buff); + var res = state.LoadBuffer(buff, LuaScriptChunkKind.Text); if (res != LuaStatus.OK) { state.ClearStack(); @@ -3071,8 +3071,8 @@ int luaArgCount private unsafe int CompileCommon(nint luaState, ref TResponse resp) where TResponse : struct, IResponseAdapter { - // 1 for function, 1 for code string - const int NeededStackSpace = 2; + // 1 for function, 1 for code string, 1 for mode string + const int NeededStackSpace = 3; Debug.Assert(functionRegistryIndex == -1, "Shouldn't compile multiple times"); @@ -3081,7 +3081,16 @@ private unsafe int CompileCommon(nint luaState, ref TResponse resp) Debug.Assert(state.TryEnsureMinimumStackCapacity(NeededStackSpace), "LUA_MIN_STACK should be high enough that this cannot happen"); _ = state.RawGetInteger(LuaType.Function, (int)LuaRegistry.Index, loadSandboxedRegistryIndex); - if (!state.TryPushBuffer(source.Span)) + var mode = source.Kind switch + { + LuaScriptChunkKind.Text => "t"u8, + LuaScriptChunkKind.GarnetGeneratedBinary => "b"u8, + LuaScriptChunkKind.TextOrBinary => "bt"u8, + _ => throw new ArgumentOutOfRangeException(nameof(source)) + }; + + if (!state.TryPushBuffer(source.Data.Span) || + !state.TryPushBuffer(mode)) { while (!RespWriteUtils.TryWriteError(CmdStrings.LUA_out_of_memory, ref resp.BufferCur, resp.BufferEnd)) resp.SendAndReset(); @@ -3089,7 +3098,7 @@ private unsafe int CompileCommon(nint luaState, ref TResponse resp) return 0; } - var callRes = state.PCall(1, 2); + var callRes = state.PCall(2, 2); // On success the stack will have two things on it: // 1. The error (nil if not error) diff --git a/libs/server/Lua/LuaRunner.Loader.cs b/libs/server/Lua/LuaRunner.Loader.cs index a9a95ef9e53..5335fb77871 100644 --- a/libs/server/Lua/LuaRunner.Loader.cs +++ b/libs/server/Lua/LuaRunner.Loader.cs @@ -314,8 +314,8 @@ function reset_keys_and_argv(fromKey, fromArgv) -- force new 'global' environment to be readonly recursively_readonly_table(sandbox_env) -- responsible for sandboxing user provided code -function load_sandboxed(source) - local rawFunc, err = load(source, nil, nil, sandbox_env) +function load_sandboxed(source, mode) + local rawFunc, err = load(source, nil, mode, sandbox_env) return err, rawFunc end @@ -448,7 +448,7 @@ internal static ReadOnlyMemory PrepareLoaderBlockBytes(HashSet all compilingState.Remove(1); - if (compilingState.LoadString(Encoding.UTF8.GetBytes(finalLoaderBlock)) != LuaStatus.OK) + if (compilingState.LoadBuffer(Encoding.UTF8.GetBytes(finalLoaderBlock), LuaScriptChunkKind.Text) != LuaStatus.OK) { throw new InvalidOperationException("Compiling function should not fail"); } @@ -479,7 +479,7 @@ internal static ReadOnlyMemory PrepareLoaderBlockBytes(HashSet all /// /// These ops are faster to load into a runtime than parsing the whole source file again. /// - internal static byte[] CompileSource(ReadOnlySpan source) + internal static LuaScriptChunk CompileSource(ReadOnlySpan source) { // This is equivalent to calling // @@ -496,10 +496,10 @@ internal static byte[] CompileSource(ReadOnlySpan source) state.Remove(1); - if (state.LoadString(source) != LuaStatus.OK) + if (state.LoadBuffer(source, LuaScriptChunkKind.Text) != LuaStatus.OK) { // If we're going to fail, just keep the source as is - a future load attempt will fail it too - return source.ToArray(); + return new(source.ToArray(), LuaScriptChunkKind.Text); } state.PushBoolean(true); @@ -507,12 +507,12 @@ internal static byte[] CompileSource(ReadOnlySpan source) if (state.PCall(2, 1) != LuaStatus.OK) { // If we're going to fail, just keep the source as is - a future load attempt will fail it too - return source.ToArray(); + return new(source.ToArray(), LuaScriptChunkKind.Text); } state.KnownStringToBuffer(1, out var ops); - return ops.ToArray(); + return new(ops.ToArray(), LuaScriptChunkKind.GarnetGeneratedBinary); } } } \ No newline at end of file diff --git a/libs/server/Lua/LuaRunner.cs b/libs/server/Lua/LuaRunner.cs index 5b68363b7af..5cc27e6be48 100644 --- a/libs/server/Lua/LuaRunner.cs +++ b/libs/server/Lua/LuaRunner.cs @@ -160,7 +160,7 @@ public void SendAndReset() readonly LuaLoggingMode logMode; readonly HashSet allowedFunctions; - readonly ReadOnlyMemory source; + readonly LuaScriptChunk source; readonly ScratchBufferNetworkSender scratchBufferNetworkSender; readonly RespServerSession respServerSession; @@ -214,6 +214,22 @@ public unsafe LuaRunner( ScratchBufferNetworkSender scratchBufferNetworkSender = null, string redisVersion = "0.0.0.0", ILogger logger = null + ) + : this(memMode, memLimitBytes, logMode, allowedFunctions, new LuaScriptChunk(source, LuaScriptChunkKind.Text), txnMode, respServerSession, scratchBufferNetworkSender, redisVersion, logger) + { + } + + internal unsafe LuaRunner( + LuaMemoryManagementMode memMode, + int? memLimitBytes, + LuaLoggingMode logMode, + HashSet allowedFunctions, + LuaScriptChunk source, + bool txnMode = false, + RespServerSession respServerSession = null, + ScratchBufferNetworkSender scratchBufferNetworkSender = null, + string redisVersion = "0.0.0.0", + ILogger logger = null ) { // KEYS and ARGV are always access by index, and to avoid allocation concerns @@ -328,7 +344,7 @@ public unsafe LuaRunner( throw new GarnetException("Insufficient space in Lua VM for redis version number global"); } - var loadRes = state.LoadBuffer(PrepareLoaderBlockBytes(allowedFunctions, logger).Span); + var loadRes = state.LoadBuffer(PrepareLoaderBlockBytes(allowedFunctions, logger).Span, LuaScriptChunkKind.GarnetGeneratedBinary); if (loadRes != LuaStatus.OK) { if (state.StackTop == 1 && state.Type(1) == LuaType.String) diff --git a/libs/server/Lua/LuaScriptHandle.cs b/libs/server/Lua/LuaScriptHandle.cs index caa9f066dfa..c747fe2db00 100644 --- a/libs/server/Lua/LuaScriptHandle.cs +++ b/libs/server/Lua/LuaScriptHandle.cs @@ -5,6 +5,15 @@ namespace Garnet.server { + internal enum LuaScriptChunkKind : byte + { + Text, + GarnetGeneratedBinary, + TextOrBinary + } + + internal readonly record struct LuaScriptChunk(ReadOnlyMemory Data, LuaScriptChunkKind Kind); + /// /// Used to track the lifetime a shared Lua script, which may end up backing multiple s. /// @@ -21,11 +30,22 @@ public sealed class LuaScriptHandle : IDisposable /// /// Source (or compiled source) for the associated Lua script. /// - public ReadOnlyMemory ScriptData { get; } + public ReadOnlyMemory ScriptData => Chunk.Data; + + internal LuaScriptChunk Chunk { get; } + /// + /// Creates a handle for Lua source or a compatible compiled chunk. + /// + /// Lua source or compiled chunk. public LuaScriptHandle(ReadOnlyMemory scriptData) + : this(new LuaScriptChunk(scriptData, LuaScriptChunkKind.TextOrBinary)) + { + } + + internal LuaScriptHandle(LuaScriptChunk chunk) { - ScriptData = scriptData; + Chunk = chunk; } /// diff --git a/libs/server/Lua/LuaStateWrapper.cs b/libs/server/Lua/LuaStateWrapper.cs index 48f9fca6203..121edb48c29 100644 --- a/libs/server/Lua/LuaStateWrapper.cs +++ b/libs/server/Lua/LuaStateWrapper.cs @@ -463,42 +463,13 @@ internal bool TrySetGlobal(ReadOnlySpan nullTerminatedGlobalName) /// Maintains and to minimize p/invoke calls. /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal LuaStatus LoadBuffer(ReadOnlySpan buffer) + internal LuaStatus LoadBuffer(ReadOnlySpan buffer, LuaScriptChunkKind chunkKind) { AssertLuaStackNotFull(2); // Note that https://www.lua.org/source/5.4/lauxlib.c.html#luaL_loadbufferx is implemented in terms of // a PCall, so we don't have to worry about crashes. - var ret = NativeMethods.LoadBuffer(state, buffer); - - if (ret != LuaStatus.OK) - { - StackTop = NativeMethods.GetTop(state); - } - else - { - UpdateStackTop(1); - } - - AssertLuaStackExpected(); - - return ret; - } - - /// - /// This should be used for all LoadStrings into Lua. - /// - /// Note that this is different from pushing or loading buffer, as the loaded buffer is compiled but NOT executed. - /// - /// Maintains and to minimize p/invoke calls. - /// - internal LuaStatus LoadString(ReadOnlySpan buffer) - { - AssertLuaStackNotFull(2); - - // Note that https://www.lua.org/source/5.4/lauxlib.h.html#luaL_loadbuffer is implemented in terms of - // a PCall, so we don't have to worry about crashes. - var ret = NativeMethods.LoadString(state, buffer); + var ret = NativeMethods.LoadBuffer(state, buffer, chunkKind); if (ret != LuaStatus.OK) { diff --git a/libs/server/Lua/NativeMethods.cs b/libs/server/Lua/NativeMethods.cs index 254b6c43e88..93196ce0582 100644 --- a/libs/server/Lua/NativeMethods.cs +++ b/libs/server/Lua/NativeMethods.cs @@ -392,24 +392,19 @@ internal static unsafe void PushBuffer(lua_State luaState, ReadOnlySpan st /// /// Provided data is copied, and can be reused once this call returns. /// - internal static unsafe LuaStatus LoadBuffer(lua_State luaState, ReadOnlySpan str) + internal static unsafe LuaStatus LoadBuffer(lua_State luaState, ReadOnlySpan str, LuaScriptChunkKind chunkKind) { - fixed (byte* ptr = str) + ReadOnlySpan mode = chunkKind switch { - return luaL_loadbufferx(luaState, (charptr_t)ptr, (size_t)str.Length, (charptr_t)UIntPtr.Zero, (charptr_t)UIntPtr.Zero); - } - } + LuaScriptChunkKind.Text => "t\0"u8, + LuaScriptChunkKind.GarnetGeneratedBinary => "b\0"u8, + _ => throw new ArgumentOutOfRangeException(nameof(chunkKind)) + }; - /// - /// Push given span to stack, and compiles it. - /// - /// Provided data is copied, and can be reused once this call returns. - /// - internal static unsafe LuaStatus LoadString(lua_State luaState, ReadOnlySpan str) - { fixed (byte* ptr = str) + fixed (byte* modePtr = mode) { - return luaL_loadstring(luaState, (charptr_t)ptr); + return luaL_loadbufferx(luaState, (charptr_t)ptr, (size_t)str.Length, (charptr_t)UIntPtr.Zero, (charptr_t)modePtr); } } diff --git a/libs/server/Lua/SessionScriptCache.cs b/libs/server/Lua/SessionScriptCache.cs index 705a0b9deaf..e5272b5dc3f 100644 --- a/libs/server/Lua/SessionScriptCache.cs +++ b/libs/server/Lua/SessionScriptCache.cs @@ -170,6 +170,7 @@ public bool TryGetFromDigest(ScriptHashKey digest, out LuaRunner scriptRunner, o internal bool TryLoad( RespServerSession session, ReadOnlySpan source, + LuaScriptChunkKind sourceKind, ScriptHashKey digest, ref LuaScriptHandle luaScriptHandle, out LuaRunner runner, @@ -185,7 +186,9 @@ out ScriptHashKey? digestOnHeap try { - var compiledSource = LuaRunner.CompileSource(source); + var compiledSource = sourceKind == LuaScriptChunkKind.Text + ? LuaRunner.CompileSource(source) + : new LuaScriptChunk(source.ToArray(), sourceKind); runner = new LuaRunner(memoryManagementMode, memoryLimitBytes, logMode, allowedFunctions, compiledSource, storeWrapper.serverOptions.LuaTransactionMode, processor, scratchBufferNetworkSender, storeWrapper.redisProtocolVersion, logger); diff --git a/test/standalone/Garnet.test.scripting/LuaScriptTests.cs b/test/standalone/Garnet.test.scripting/LuaScriptTests.cs index 55d3f728887..918303b1442 100644 --- a/test/standalone/Garnet.test.scripting/LuaScriptTests.cs +++ b/test/standalone/Garnet.test.scripting/LuaScriptTests.cs @@ -1027,6 +1027,85 @@ public void MultiSessionScriptFlush() ClassicAssert.True(exc2.Message.StartsWith("NOSCRIPT ")); } + [Test] + public void EvalRequiresTextSource() + { + using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + var db = redis.GetDatabase(0); + var compiledChunk = LuaRunner.CompileSource("return 1"u8); + + ClassicAssert.AreEqual(LuaScriptChunkKind.GarnetGeneratedBinary, compiledChunk.Kind); + + var exc = ClassicAssert.Throws(() => db.Execute("EVAL", [compiledChunk.Data.ToArray(), 0])); + StringAssert.Contains("binary chunk", exc.Message); + } + + [Test] + public void ScriptLoadRequiresTextSource() + { + using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + var db = redis.GetDatabase(0); + var compiledChunk = LuaRunner.CompileSource("return 1"u8); + var hashBytes = SHA1.HashData(compiledChunk.Data.Span); + var hash = string.Join("", hashBytes.Select(static x => $"{x:x2}")); + + var exc = ClassicAssert.Throws(() => db.Execute("SCRIPT", ["LOAD", compiledChunk.Data.ToArray()])); + StringAssert.Contains("binary chunk", exc.Message); + + var exists = (RedisResult[])db.Execute("SCRIPT", ["EXISTS", hash]); + ClassicAssert.AreEqual(0, (int)exists[0]); + } + + [Test] + public void ScriptLoadEvalShaAcrossSessions() + { + using var redis1 = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + using var redis2 = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + + var db1 = redis1.GetDatabase(0); + var db2 = redis2.GetDatabase(0); + + var hash = (string)db1.Execute("SCRIPT", "LOAD", "return ARGV[1]"); + var result = (string)db2.Execute("EVALSHA", hash, 0, "value"); + + ClassicAssert.AreEqual("value", result); + } + + [Test] + public void ScriptCacheHandlesSupportTextAndCompiledChunks() + { + using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + var db = redis.GetDatabase(0); + + AddScript("return 1"u8, "return 1"u8, 1); + + var source = "return 2"u8; + var compiledChunk = LuaRunner.CompileSource(source); + ClassicAssert.AreEqual(LuaScriptChunkKind.GarnetGeneratedBinary, compiledChunk.Kind); + AddScript(source, compiledChunk.Data.Span, 2); + + void AddScript(ReadOnlySpan source, ReadOnlySpan scriptData, int expected) + { + var hash = Convert.ToHexString(SHA1.HashData(source)).ToLowerInvariant(); + var digest = GC.AllocateUninitializedArray(SessionScriptCache.SHA1Len, pinned: true); + _ = Encoding.ASCII.GetBytes(hash, digest); + + ClassicAssert.True(server.Provider.StoreWrapper.storeScriptCache.TryAdd(new ScriptHashKey(digest), new LuaScriptHandle(scriptData.ToArray()))); + ClassicAssert.AreEqual(expected, (int)db.Execute("EVALSHA", hash, 0)); + } + } + + [Test] + public void EvalUsesFullSourceLength() + { + using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + var db = redis.GetDatabase(0); + var source = Encoding.UTF8.GetBytes("return 1\0return 2"); + + var exc = ClassicAssert.Throws(() => db.Execute("EVAL", [source, 0])); + StringAssert.StartsWith("Compilation error:", exc.Message); + } + [Test] public void CrossSessionEvalScriptCaching() { From f24898082718f75c0014832565660c90df958b35 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Tue, 8 Sep 2026 15:04:13 -0700 Subject: [PATCH 2/2] Simplify Lua script source handling Keep application scripts as source text throughout caching and loading while retaining the private precompiled loader bootstrap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Lua/LuaScriptCacheOperations.cs | 4 +- libs/server/Lua/LuaCommands.cs | 6 +- libs/server/Lua/LuaRunner.Functions.cs | 19 ++--- libs/server/Lua/LuaRunner.Loader.cs | 48 +------------ libs/server/Lua/LuaRunner.cs | 20 +----- libs/server/Lua/LuaScriptHandle.cs | 26 ++----- libs/server/Lua/LuaStateWrapper.cs | 36 +++++++--- libs/server/Lua/NativeMethods.cs | 32 +++++---- libs/server/Lua/SessionScriptCache.cs | 9 +-- .../Garnet.test.scripting/LuaScriptTests.cs | 70 +++++++------------ 10 files changed, 92 insertions(+), 178 deletions(-) diff --git a/benchmark/BDN.benchmark/Lua/LuaScriptCacheOperations.cs b/benchmark/BDN.benchmark/Lua/LuaScriptCacheOperations.cs index 9bdb5707d83..72558d01f1c 100644 --- a/benchmark/BDN.benchmark/Lua/LuaScriptCacheOperations.cs +++ b/benchmark/BDN.benchmark/Lua/LuaScriptCacheOperations.cs @@ -82,7 +82,7 @@ public void IterationSetup() // Make outer hit available for every iteration LuaScriptHandle scriptHandle = null; - if (!sessionScriptCache.TryLoad(session, "return 1"u8, LuaScriptChunkKind.Text, new(outerHitDigest), ref scriptHandle, out _, out _)) + if (!sessionScriptCache.TryLoad(session, "return 1"u8, new(outerHitDigest), ref scriptHandle, out _, out _)) { throw new InvalidOperationException("Should have been able to load"); } @@ -148,7 +148,7 @@ private void LoadScript(Span digest) if (storeWrapper.storeScriptCache.TryGetValue(digestKey, out var scriptHandle)) { LuaScriptHandle newScriptHandle = null; - if (!sessionScriptCache.TryLoad(session, scriptHandle.ScriptData.Span, scriptHandle.Chunk.Kind, digestKey, ref newScriptHandle, out runner, out _)) + if (!sessionScriptCache.TryLoad(session, scriptHandle.ScriptData.Span, digestKey, ref newScriptHandle, out runner, out _)) { // TryLoad will have written an error out, it any diff --git a/libs/server/Lua/LuaCommands.cs b/libs/server/Lua/LuaCommands.cs index 9c57404e37e..4a37d88e902 100644 --- a/libs/server/Lua/LuaCommands.cs +++ b/libs/server/Lua/LuaCommands.cs @@ -48,7 +48,7 @@ private unsafe bool TryEVALSHA() { if (storeWrapper.storeScriptCache.TryGetValue(scriptKey, out var globalScriptHandle)) { - if (!sessionScriptCache.TryLoad(this, globalScriptHandle.ScriptData.Span, globalScriptHandle.Chunk.Kind, scriptKey, ref globalScriptHandle, out runner, out _)) + if (!sessionScriptCache.TryLoad(this, globalScriptHandle.ScriptData.Span, scriptKey, ref globalScriptHandle, out runner, out _)) { // TryLoad will have written an error out, it any // @@ -118,7 +118,7 @@ private unsafe bool TryEVAL() var sessionScriptHandle = globalScriptHandle; - if (!sessionScriptCache.TryLoad(this, script.ReadOnlySpan, LuaScriptChunkKind.Text, onStackScriptKey, ref sessionScriptHandle, out var runner, out var digestOnHeap)) + if (!sessionScriptCache.TryLoad(this, script.ReadOnlySpan, onStackScriptKey, ref sessionScriptHandle, out var runner, out var digestOnHeap)) { // TryLoad will have written any errors out return true; @@ -278,7 +278,7 @@ private bool NetworkScriptLoad() _ = storeWrapper.storeScriptCache.TryGetValue(onStackScriptHashKey, out var globalScriptHandle); var sessionScriptHandle = globalScriptHandle; - if (sessionScriptCache.TryLoad(this, source.ReadOnlySpan, LuaScriptChunkKind.Text, onStackScriptHashKey, ref sessionScriptHandle, out _, out var digestOnHeap)) + if (sessionScriptCache.TryLoad(this, source.ReadOnlySpan, onStackScriptHashKey, ref sessionScriptHandle, out _, out var digestOnHeap)) { // TryLoad will write any errors out diff --git a/libs/server/Lua/LuaRunner.Functions.cs b/libs/server/Lua/LuaRunner.Functions.cs index 42f85ed02ec..aca94100551 100644 --- a/libs/server/Lua/LuaRunner.Functions.cs +++ b/libs/server/Lua/LuaRunner.Functions.cs @@ -852,7 +852,7 @@ internal int LoadString(nint luaStatePtr) return LuaWrappedError(1, constStrs.InsufficientLuaStackSpace); } - var res = state.LoadBuffer(buff, LuaScriptChunkKind.Text); + var res = state.LoadTextBuffer(buff); if (res != LuaStatus.OK) { state.ClearStack(); @@ -3071,8 +3071,8 @@ int luaArgCount private unsafe int CompileCommon(nint luaState, ref TResponse resp) where TResponse : struct, IResponseAdapter { - // 1 for function, 1 for code string, 1 for mode string - const int NeededStackSpace = 3; + // 1 for function, 1 for code string + const int NeededStackSpace = 2; Debug.Assert(functionRegistryIndex == -1, "Shouldn't compile multiple times"); @@ -3081,16 +3081,7 @@ private unsafe int CompileCommon(nint luaState, ref TResponse resp) Debug.Assert(state.TryEnsureMinimumStackCapacity(NeededStackSpace), "LUA_MIN_STACK should be high enough that this cannot happen"); _ = state.RawGetInteger(LuaType.Function, (int)LuaRegistry.Index, loadSandboxedRegistryIndex); - var mode = source.Kind switch - { - LuaScriptChunkKind.Text => "t"u8, - LuaScriptChunkKind.GarnetGeneratedBinary => "b"u8, - LuaScriptChunkKind.TextOrBinary => "bt"u8, - _ => throw new ArgumentOutOfRangeException(nameof(source)) - }; - - if (!state.TryPushBuffer(source.Data.Span) || - !state.TryPushBuffer(mode)) + if (!state.TryPushBuffer(source.Span)) { while (!RespWriteUtils.TryWriteError(CmdStrings.LUA_out_of_memory, ref resp.BufferCur, resp.BufferEnd)) resp.SendAndReset(); @@ -3098,7 +3089,7 @@ private unsafe int CompileCommon(nint luaState, ref TResponse resp) return 0; } - var callRes = state.PCall(2, 2); + var callRes = state.PCall(1, 2); // On success the stack will have two things on it: // 1. The error (nil if not error) diff --git a/libs/server/Lua/LuaRunner.Loader.cs b/libs/server/Lua/LuaRunner.Loader.cs index 5335fb77871..a50143935f4 100644 --- a/libs/server/Lua/LuaRunner.Loader.cs +++ b/libs/server/Lua/LuaRunner.Loader.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.Linq; using System.Text; using KeraLua; @@ -314,8 +313,8 @@ function reset_keys_and_argv(fromKey, fromArgv) -- force new 'global' environment to be readonly recursively_readonly_table(sandbox_env) -- responsible for sandboxing user provided code -function load_sandboxed(source, mode) - local rawFunc, err = load(source, nil, mode, sandbox_env) +function load_sandboxed(source) + local rawFunc, err = load(source, nil, 't', sandbox_env) return err, rawFunc end @@ -448,7 +447,7 @@ internal static ReadOnlyMemory PrepareLoaderBlockBytes(HashSet all compilingState.Remove(1); - if (compilingState.LoadBuffer(Encoding.UTF8.GetBytes(finalLoaderBlock), LuaScriptChunkKind.Text) != LuaStatus.OK) + if (compilingState.LoadTextBuffer(Encoding.UTF8.GetBytes(finalLoaderBlock)) != LuaStatus.OK) { throw new InvalidOperationException("Compiling function should not fail"); } @@ -473,46 +472,5 @@ internal static ReadOnlyMemory PrepareLoaderBlockBytes(HashSet all return newCache.LoaderBlockBytes; } - - /// - /// Take a chunk of Lua code and convert it to binary ops. - /// - /// These ops are faster to load into a runtime than parsing the whole source file again. - /// - internal static LuaScriptChunk CompileSource(ReadOnlySpan source) - { - // This is equivalent to calling - // - // string.dump(, true) - // - // Which gives us the opcode version of source on the stack - - using var state = new LuaStateWrapper(LuaMemoryManagementMode.Native, null, null); - - state.GetGlobal(LuaType.Table, "string\0"u8); - var pushRes = state.TryPushBuffer("dump"u8); - Debug.Assert(pushRes, "Pushing 'dump' should never fail"); - _ = state.RawGet(LuaType.Function, 1); - - state.Remove(1); - - if (state.LoadBuffer(source, LuaScriptChunkKind.Text) != LuaStatus.OK) - { - // If we're going to fail, just keep the source as is - a future load attempt will fail it too - return new(source.ToArray(), LuaScriptChunkKind.Text); - } - - state.PushBoolean(true); - - if (state.PCall(2, 1) != LuaStatus.OK) - { - // If we're going to fail, just keep the source as is - a future load attempt will fail it too - return new(source.ToArray(), LuaScriptChunkKind.Text); - } - - state.KnownStringToBuffer(1, out var ops); - - return new(ops.ToArray(), LuaScriptChunkKind.GarnetGeneratedBinary); - } } } \ No newline at end of file diff --git a/libs/server/Lua/LuaRunner.cs b/libs/server/Lua/LuaRunner.cs index 5cc27e6be48..89e26cf7d2f 100644 --- a/libs/server/Lua/LuaRunner.cs +++ b/libs/server/Lua/LuaRunner.cs @@ -160,7 +160,7 @@ public void SendAndReset() readonly LuaLoggingMode logMode; readonly HashSet allowedFunctions; - readonly LuaScriptChunk source; + readonly ReadOnlyMemory source; readonly ScratchBufferNetworkSender scratchBufferNetworkSender; readonly RespServerSession respServerSession; @@ -214,22 +214,6 @@ public unsafe LuaRunner( ScratchBufferNetworkSender scratchBufferNetworkSender = null, string redisVersion = "0.0.0.0", ILogger logger = null - ) - : this(memMode, memLimitBytes, logMode, allowedFunctions, new LuaScriptChunk(source, LuaScriptChunkKind.Text), txnMode, respServerSession, scratchBufferNetworkSender, redisVersion, logger) - { - } - - internal unsafe LuaRunner( - LuaMemoryManagementMode memMode, - int? memLimitBytes, - LuaLoggingMode logMode, - HashSet allowedFunctions, - LuaScriptChunk source, - bool txnMode = false, - RespServerSession respServerSession = null, - ScratchBufferNetworkSender scratchBufferNetworkSender = null, - string redisVersion = "0.0.0.0", - ILogger logger = null ) { // KEYS and ARGV are always access by index, and to avoid allocation concerns @@ -344,7 +328,7 @@ internal unsafe LuaRunner( throw new GarnetException("Insufficient space in Lua VM for redis version number global"); } - var loadRes = state.LoadBuffer(PrepareLoaderBlockBytes(allowedFunctions, logger).Span, LuaScriptChunkKind.GarnetGeneratedBinary); + var loadRes = state.LoadBinaryBuffer(PrepareLoaderBlockBytes(allowedFunctions, logger).Span); if (loadRes != LuaStatus.OK) { if (state.StackTop == 1 && state.Type(1) == LuaType.String) diff --git a/libs/server/Lua/LuaScriptHandle.cs b/libs/server/Lua/LuaScriptHandle.cs index c747fe2db00..c2793b6e58b 100644 --- a/libs/server/Lua/LuaScriptHandle.cs +++ b/libs/server/Lua/LuaScriptHandle.cs @@ -5,15 +5,6 @@ namespace Garnet.server { - internal enum LuaScriptChunkKind : byte - { - Text, - GarnetGeneratedBinary, - TextOrBinary - } - - internal readonly record struct LuaScriptChunk(ReadOnlyMemory Data, LuaScriptChunkKind Kind); - /// /// Used to track the lifetime a shared Lua script, which may end up backing multiple s. /// @@ -28,24 +19,17 @@ public sealed class LuaScriptHandle : IDisposable public bool IsDisposed { get; private set; } /// - /// Source (or compiled source) for the associated Lua script. + /// Source for the associated Lua script. /// - public ReadOnlyMemory ScriptData => Chunk.Data; - - internal LuaScriptChunk Chunk { get; } + public ReadOnlyMemory ScriptData { get; } /// - /// Creates a handle for Lua source or a compatible compiled chunk. + /// Creates a handle for Lua source. /// - /// Lua source or compiled chunk. + /// Lua source. public LuaScriptHandle(ReadOnlyMemory scriptData) - : this(new LuaScriptChunk(scriptData, LuaScriptChunkKind.TextOrBinary)) - { - } - - internal LuaScriptHandle(LuaScriptChunk chunk) { - Chunk = chunk; + ScriptData = scriptData; } /// diff --git a/libs/server/Lua/LuaStateWrapper.cs b/libs/server/Lua/LuaStateWrapper.cs index 121edb48c29..3148dba374d 100644 --- a/libs/server/Lua/LuaStateWrapper.cs +++ b/libs/server/Lua/LuaStateWrapper.cs @@ -456,20 +456,38 @@ internal bool TrySetGlobal(ReadOnlySpan nullTerminatedGlobalName) } /// - /// This should be used for all LoadBuffers into Lua. - /// - /// Note that this is different from pushing a buffer, as the loaded buffer is compiled and executed. - /// - /// Maintains and to minimize p/invoke calls. + /// Loads Garnet's precompiled loader buffer into Lua. + /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal LuaStatus LoadBinaryBuffer(ReadOnlySpan buffer) + { + AssertLuaStackNotFull(2); + + var ret = NativeMethods.LoadBinaryBuffer(state, buffer); + + if (ret != LuaStatus.OK) + { + StackTop = NativeMethods.GetTop(state); + } + else + { + UpdateStackTop(1); + } + + AssertLuaStackExpected(); + + return ret; + } + + /// + /// Loads an exact-length text buffer into Lua. /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal LuaStatus LoadBuffer(ReadOnlySpan buffer, LuaScriptChunkKind chunkKind) + internal LuaStatus LoadTextBuffer(ReadOnlySpan buffer) { AssertLuaStackNotFull(2); - // Note that https://www.lua.org/source/5.4/lauxlib.c.html#luaL_loadbufferx is implemented in terms of - // a PCall, so we don't have to worry about crashes. - var ret = NativeMethods.LoadBuffer(state, buffer, chunkKind); + var ret = NativeMethods.LoadTextBuffer(state, buffer); if (ret != LuaStatus.OK) { diff --git a/libs/server/Lua/NativeMethods.cs b/libs/server/Lua/NativeMethods.cs index 93196ce0582..7e7e17e17b1 100644 --- a/libs/server/Lua/NativeMethods.cs +++ b/libs/server/Lua/NativeMethods.cs @@ -45,13 +45,6 @@ internal static partial class NativeMethods [UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])] private static partial LuaStatus luaL_loadbufferx(lua_State luaState, charptr_t buff, size_t sz, charptr_t name, charptr_t mode); - /// - /// see: https://www.lua.org/manual/5.4/manual.html#luaL_loadstring - /// - [LibraryImport(LuaLibraryName)] - [UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])] - private static partial LuaStatus luaL_loadstring(lua_State lua_State, charptr_t buff); - /// /// see: https://www.lua.org/manual/5.4/manual.html#luaL_newstate /// @@ -388,18 +381,27 @@ internal static unsafe void PushBuffer(lua_State luaState, ReadOnlySpan st } /// - /// Push given span to stack, compiles it, and executes it. - /// + /// Loads Garnet's precompiled loader buffer. /// Provided data is copied, and can be reused once this call returns. /// - internal static unsafe LuaStatus LoadBuffer(lua_State luaState, ReadOnlySpan str, LuaScriptChunkKind chunkKind) + internal static unsafe LuaStatus LoadBinaryBuffer(lua_State luaState, ReadOnlySpan str) { - ReadOnlySpan mode = chunkKind switch + ReadOnlySpan mode = "b\0"u8; + + fixed (byte* ptr = str) + fixed (byte* modePtr = mode) { - LuaScriptChunkKind.Text => "t\0"u8, - LuaScriptChunkKind.GarnetGeneratedBinary => "b\0"u8, - _ => throw new ArgumentOutOfRangeException(nameof(chunkKind)) - }; + return luaL_loadbufferx(luaState, (charptr_t)ptr, (size_t)str.Length, (charptr_t)UIntPtr.Zero, (charptr_t)modePtr); + } + } + + /// + /// Loads an exact-length text buffer. + /// Provided data is copied, and can be reused once this call returns. + /// + internal static unsafe LuaStatus LoadTextBuffer(lua_State luaState, ReadOnlySpan str) + { + ReadOnlySpan mode = "t\0"u8; fixed (byte* ptr = str) fixed (byte* modePtr = mode) diff --git a/libs/server/Lua/SessionScriptCache.cs b/libs/server/Lua/SessionScriptCache.cs index e5272b5dc3f..a08208b58e8 100644 --- a/libs/server/Lua/SessionScriptCache.cs +++ b/libs/server/Lua/SessionScriptCache.cs @@ -170,7 +170,6 @@ public bool TryGetFromDigest(ScriptHashKey digest, out LuaRunner scriptRunner, o internal bool TryLoad( RespServerSession session, ReadOnlySpan source, - LuaScriptChunkKind sourceKind, ScriptHashKey digest, ref LuaScriptHandle luaScriptHandle, out LuaRunner runner, @@ -186,11 +185,9 @@ out ScriptHashKey? digestOnHeap try { - var compiledSource = sourceKind == LuaScriptChunkKind.Text - ? LuaRunner.CompileSource(source) - : new LuaScriptChunk(source.ToArray(), sourceKind); + var scriptData = luaScriptHandle?.ScriptData ?? source.ToArray(); - runner = new LuaRunner(memoryManagementMode, memoryLimitBytes, logMode, allowedFunctions, compiledSource, storeWrapper.serverOptions.LuaTransactionMode, processor, scratchBufferNetworkSender, storeWrapper.redisProtocolVersion, logger); + runner = new LuaRunner(memoryManagementMode, memoryLimitBytes, logMode, allowedFunctions, scriptData, storeWrapper.serverOptions.LuaTransactionMode, processor, scratchBufferNetworkSender, storeWrapper.redisProtocolVersion, logger); // If compilation fails, an error is written out if (runner.CompileForSession(session)) @@ -206,7 +203,7 @@ out ScriptHashKey? digestOnHeap ScriptHashKey storeKeyDigest = new(into); digestOnHeap = storeKeyDigest; - luaScriptHandle ??= new(compiledSource); + luaScriptHandle ??= new(scriptData); scriptCache.Add(storeKeyDigest, (runner, luaScriptHandle)); // On first script load, register for timeout notifications diff --git a/test/standalone/Garnet.test.scripting/LuaScriptTests.cs b/test/standalone/Garnet.test.scripting/LuaScriptTests.cs index 918303b1442..e66d85455e8 100644 --- a/test/standalone/Garnet.test.scripting/LuaScriptTests.cs +++ b/test/standalone/Garnet.test.scripting/LuaScriptTests.cs @@ -16,6 +16,7 @@ using System.Threading.Tasks; using Garnet.common; using Garnet.server; +using KeraLua; using NUnit.Framework; using NUnit.Framework.Legacy; using StackExchange.Redis; @@ -1028,71 +1029,50 @@ public void MultiSessionScriptFlush() } [Test] - public void EvalRequiresTextSource() + public void ScriptInputsRequireTextSource() { using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); var db = redis.GetDatabase(0); - var compiledChunk = LuaRunner.CompileSource("return 1"u8); + var binaryChunk = CompileChunk("return 1"u8); - ClassicAssert.AreEqual(LuaScriptChunkKind.GarnetGeneratedBinary, compiledChunk.Kind); - - var exc = ClassicAssert.Throws(() => db.Execute("EVAL", [compiledChunk.Data.ToArray(), 0])); - StringAssert.Contains("binary chunk", exc.Message); - } - - [Test] - public void ScriptLoadRequiresTextSource() - { - using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); - var db = redis.GetDatabase(0); - var compiledChunk = LuaRunner.CompileSource("return 1"u8); - var hashBytes = SHA1.HashData(compiledChunk.Data.Span); - var hash = string.Join("", hashBytes.Select(static x => $"{x:x2}")); - - var exc = ClassicAssert.Throws(() => db.Execute("SCRIPT", ["LOAD", compiledChunk.Data.ToArray()])); - StringAssert.Contains("binary chunk", exc.Message); + var evalException = ClassicAssert.Throws(() => db.Execute("EVAL", [binaryChunk, 0])); + StringAssert.Contains("binary chunk", evalException.Message); + var hash = Convert.ToHexString(SHA1.HashData(binaryChunk)).ToLowerInvariant(); + var loadException = ClassicAssert.Throws(() => db.Execute("SCRIPT", ["LOAD", binaryChunk])); + StringAssert.Contains("binary chunk", loadException.Message); var exists = (RedisResult[])db.Execute("SCRIPT", ["EXISTS", hash]); ClassicAssert.AreEqual(0, (int)exists[0]); } - [Test] - public void ScriptLoadEvalShaAcrossSessions() + private static byte[] CompileChunk(ReadOnlySpan source) { - using var redis1 = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); - using var redis2 = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); - - var db1 = redis1.GetDatabase(0); - var db2 = redis2.GetDatabase(0); + using var state = new LuaStateWrapper(LuaMemoryManagementMode.Native, null, null); - var hash = (string)db1.Execute("SCRIPT", "LOAD", "return ARGV[1]"); - var result = (string)db2.Execute("EVALSHA", hash, 0, "value"); + state.GetGlobal(LuaType.Table, "string\0"u8); + ClassicAssert.True(state.TryPushBuffer("dump"u8)); + _ = state.RawGet(LuaType.Function, 1); + state.Remove(1); + ClassicAssert.AreEqual(LuaStatus.OK, state.LoadTextBuffer(source)); + state.PushBoolean(true); + ClassicAssert.AreEqual(LuaStatus.OK, state.PCall(2, 1)); + state.KnownStringToBuffer(1, out var chunk); - ClassicAssert.AreEqual("value", result); + return chunk.ToArray(); } [Test] - public void ScriptCacheHandlesSupportTextAndCompiledChunks() + public void HostInsertedScriptSource() { using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); var db = redis.GetDatabase(0); - - AddScript("return 1"u8, "return 1"u8, 1); - var source = "return 2"u8; - var compiledChunk = LuaRunner.CompileSource(source); - ClassicAssert.AreEqual(LuaScriptChunkKind.GarnetGeneratedBinary, compiledChunk.Kind); - AddScript(source, compiledChunk.Data.Span, 2); + var hash = Convert.ToHexString(SHA1.HashData(source)).ToLowerInvariant(); + var digest = GC.AllocateUninitializedArray(SessionScriptCache.SHA1Len, pinned: true); + _ = Encoding.ASCII.GetBytes(hash, digest); - void AddScript(ReadOnlySpan source, ReadOnlySpan scriptData, int expected) - { - var hash = Convert.ToHexString(SHA1.HashData(source)).ToLowerInvariant(); - var digest = GC.AllocateUninitializedArray(SessionScriptCache.SHA1Len, pinned: true); - _ = Encoding.ASCII.GetBytes(hash, digest); - - ClassicAssert.True(server.Provider.StoreWrapper.storeScriptCache.TryAdd(new ScriptHashKey(digest), new LuaScriptHandle(scriptData.ToArray()))); - ClassicAssert.AreEqual(expected, (int)db.Execute("EVALSHA", hash, 0)); - } + ClassicAssert.True(server.Provider.StoreWrapper.storeScriptCache.TryAdd(new ScriptHashKey(digest), new LuaScriptHandle(source.ToArray()))); + ClassicAssert.AreEqual(2, (int)db.Execute("EVALSHA", hash, 0)); } [Test]