Enforce text-only loading for Lua script input

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.LoadTextBuffer(buff);
             if (res != LuaStatus.OK)
             {
                 state.ClearStack();

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;
@@ -315,7 +314,7 @@ function reset_keys_and_argv(fromKey, fromArgv)
 recursively_readonly_table(sandbox_env)
 -- responsible for sandboxing user provided code
 function load_sandboxed(source)
-    local rawFunc, err = load(source, nil, nil, sandbox_env)
+    local rawFunc, err = load(source, nil, 't', sandbox_env)
 
     return err, rawFunc
 end
@@ -448,7 +447,7 @@ internal static ReadOnlyMemory<byte> PrepareLoaderBlockBytes(HashSet<string> all
 
                 compilingState.Remove(1);
 
-                if (compilingState.LoadString(Encoding.UTF8.GetBytes(finalLoaderBlock)) != 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<byte> PrepareLoaderBlockBytes(HashSet<string> all
 
             return newCache.LoaderBlockBytes;
         }
-
-        /// <summary>
-        /// 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.
-        /// </summary>
-        internal static byte[] CompileSource(ReadOnlySpan<byte> source)
-        {
-            // This is equivalent to calling 
-            //
-            // string.dump(<function equivalent to source>, 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.LoadString(source) != 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();
-            }
-
-            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 source.ToArray();
-            }
-
-            state.KnownStringToBuffer(1, out var ops);
-
-            return ops.ToArray();
-        }
     }
 }

libs/server/Lua/LuaRunner.cs

@@ -328,7 +328,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.LoadBinaryBuffer(PrepareLoaderBlockBytes(allowedFunctions, logger).Span);
             if (loadRes != LuaStatus.OK)
             {
                 if (state.StackTop == 1 && state.Type(1) == LuaType.String)

libs/server/Lua/LuaScriptHandle.cs

@@ -19,10 +19,14 @@ public sealed class LuaScriptHandle : IDisposable
         public bool IsDisposed { get; private set; }
 
         /// <summary>
-        /// Source (or compiled source) for the associated Lua script.
+        /// Source for the associated Lua script.
         /// </summary>
         public ReadOnlyMemory<byte> ScriptData { get; }
 
+        /// <summary>
+        /// Creates a handle for Lua source.
+        /// </summary>
+        /// <param name="scriptData">Lua source.</param>
         public LuaScriptHandle(ReadOnlyMemory<byte> scriptData)
         {
             ScriptData = scriptData;

libs/server/Lua/LuaStateWrapper.cs

@@ -456,20 +456,14 @@ internal bool TrySetGlobal(ReadOnlySpan<byte> nullTerminatedGlobalName)
         }
 
         /// <summary>
-        /// 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 <see cref="curStackSize"/> and <see cref="StackTop"/> to minimize p/invoke calls.
+        /// Loads Garnet's precompiled loader buffer into Lua.
         /// </summary>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal LuaStatus LoadBuffer(ReadOnlySpan<byte> buffer)
+        internal LuaStatus LoadBinaryBuffer(ReadOnlySpan<byte> 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);
+            var ret = NativeMethods.LoadBinaryBuffer(state, buffer);
 
             if (ret != LuaStatus.OK)
             {
@@ -486,19 +480,14 @@ internal LuaStatus LoadBuffer(ReadOnlySpan<byte> buffer)
         }
 
         /// <summary>
-        /// 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 <see cref="curStackSize"/> and <see cref="StackTop"/> to minimize p/invoke calls.
+        /// Loads an exact-length text buffer into Lua.
         /// </summary>
-        internal LuaStatus LoadString(ReadOnlySpan<byte> buffer)
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        internal LuaStatus LoadTextBuffer(ReadOnlySpan<byte> 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.LoadTextBuffer(state, buffer);
 
             if (ret != LuaStatus.OK)
             {

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);
 
-        /// <summary>
-        /// see: https://www.lua.org/manual/5.4/manual.html#luaL_loadstring
-        /// </summary>
-        [LibraryImport(LuaLibraryName)]
-        [UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])]
-        private static partial LuaStatus luaL_loadstring(lua_State lua_State, charptr_t buff);
-
         /// <summary>
         /// see: https://www.lua.org/manual/5.4/manual.html#luaL_newstate
         /// </summary>
@@ -388,28 +381,32 @@ internal static unsafe void PushBuffer(lua_State luaState, ReadOnlySpan<byte> st
         }
 
         /// <summary>
-        /// 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.
         /// </summary>
-        internal static unsafe LuaStatus LoadBuffer(lua_State luaState, ReadOnlySpan<byte> str)
+        internal static unsafe LuaStatus LoadBinaryBuffer(lua_State luaState, ReadOnlySpan<byte> str)
         {
+            ReadOnlySpan<byte> mode = "b\0"u8;
+
             fixed (byte* ptr = str)
+            fixed (byte* modePtr = mode)
             {
-                return luaL_loadbufferx(luaState, (charptr_t)ptr, (size_t)str.Length, (charptr_t)UIntPtr.Zero, (charptr_t)UIntPtr.Zero);
+                return luaL_loadbufferx(luaState, (charptr_t)ptr, (size_t)str.Length, (charptr_t)UIntPtr.Zero, (charptr_t)modePtr);
             }
         }
 
         /// <summary>
-        /// Push given span to stack, and compiles it.
-        /// 
+        /// Loads an exact-length text buffer.
         /// Provided data is copied, and can be reused once this call returns.
         /// </summary>
-        internal static unsafe LuaStatus LoadString(lua_State luaState, ReadOnlySpan<byte> str)
+        internal static unsafe LuaStatus LoadTextBuffer(lua_State luaState, ReadOnlySpan<byte> str)
         {
+            ReadOnlySpan<byte> mode = "t\0"u8;
+
             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);
             }
         }
 

libs/server/Lua/SessionScriptCache.cs

@@ -185,9 +185,9 @@ out ScriptHashKey? digestOnHeap
 
             try
             {
-                var compiledSource = LuaRunner.CompileSource(source);
+                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))
@@ -203,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

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;
@@ -1027,6 +1028,64 @@ public void MultiSessionScriptFlush()
             ClassicAssert.True(exc2.Message.StartsWith("NOSCRIPT "));
         }
 
+        [Test]
+        public void ScriptInputsRequireTextSource()
+        {
+            using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
+            var db = redis.GetDatabase(0);
+            var binaryChunk = CompileChunk("return 1"u8);
+
+            var evalException = ClassicAssert.Throws<RedisServerException>(() => db.Execute("EVAL", [binaryChunk, 0]));
+            StringAssert.Contains("binary chunk", evalException.Message);
+
+            var hash = Convert.ToHexString(SHA1.HashData(binaryChunk)).ToLowerInvariant();
+            var loadException = ClassicAssert.Throws<RedisServerException>(() => 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]);
+        }
+
+        private static byte[] CompileChunk(ReadOnlySpan<byte> source)
+        {
+            using var state = new LuaStateWrapper(LuaMemoryManagementMode.Native, null, null);
+
+            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);
+
+            return chunk.ToArray();
+        }
+
+        [Test]
+        public void HostInsertedScriptSource()
+        {
+            using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
+            var db = redis.GetDatabase(0);
+            var source = "return 2"u8;
+            var hash = Convert.ToHexString(SHA1.HashData(source)).ToLowerInvariant();
+            var digest = GC.AllocateUninitializedArray<byte>(SessionScriptCache.SHA1Len, pinned: true);
+            _ = Encoding.ASCII.GetBytes(hash, digest);
+
+            ClassicAssert.True(server.Provider.StoreWrapper.storeScriptCache.TryAdd(new ScriptHashKey(digest), new LuaScriptHandle(source.ToArray())));
+            ClassicAssert.AreEqual(2, (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<RedisServerException>(() => db.Execute("EVAL", [source, 0]));
+            StringAssert.StartsWith("Compilation error:", exc.Message);
+        }
+
         [Test]
         public void CrossSessionEvalScriptCaching()
         {