Implement remaining Lua runtime libraries #1075
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… and we can configure it reasonably; implements a number of workarounds to accomplish this, as the 5.1 <-> 5.4 Lua version difference also had some library changes
…re pretty sloppy, so ran tests against Redis first to verify matching behavior
…a superset of what Redis wants; technically we allow more than Redis does, but I think that's a reasonable tradeoff for implementation complexity
…exception tests on Linux until that refactor happens
There was a problem hiding this comment.
Pull Request Overview
This PR implements the remaining Lua runtime libraries and extends the allowlist configuration for imported Lua functions. Key changes include:
- Adding support for various Lua library functions (cjson, cmsgpack, bit, math, struct).
- Extending the configuration options (LuaAllowedFunctions) and updating corresponding tests.
- Updating native and managed Lua APIs and associated benchmarks.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Garnet.test/GarnetServerConfigTests.cs | Updated test comments to reflect JSON args usage |
| test/Garnet.test/LuaScriptRunnerTests.cs | Adjusted error message expectations and added tests for allowed functions |
| libs/server/ArgSlice/ScratchBufferManager.cs | Added a UTF8EncodeString method to better handle string encoding in the scratch buffer |
| libs/host/Configuration/Options.cs | Extended Options to support allowed Lua functions via a new HashSet property |
| libs/server/Lua/LuaStateWrapper.cs | Introduced a Remove() method and improved documentation on load functions |
| libs/server/Lua/LuaOptions.cs | Updated the constructor to accept allowed functions and initialize them appropriately |
| benchmark/BDN.benchmark/Operations/OperationsBase.cs | Updated LuaOptions instantiation to include allowedFunctions parameter |
| libs/server/Lua/SessionScriptCache.cs | Passed allowedFunctions to LuaRunner to enforce function restrictions |
| benchmark/BDN.benchmark/Lua/LuaParams.cs | Revised options creation for benchmarks with allowed functions |
| test/Garnet.test/TestUtils.cs | Modified test helpers to accept allowed Lua functions |
| libs/server/Lua/NativeMethods.cs | Enhanced native method documentation and updated methods to reflect execution changes in load functions |
Co-authored-by: Copilot <[email protected]>
…s; consolidate condition checks; restore non-error-y parts of tests in .NET 9 and non-Windows builds
badrishc
approved these changes
Mar 18, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes:
LuaAllowedFunctions)cjson.encodeandcjson.decodebit.*functionsmath.*functionscmsgpack.packandcmsgpack.unpackstruct.pack,struct.unpack, andstruct.sizeTODOs:
math.*functionsList<byte>orbyte[]instances and instead useScratchBufferManagerWhile large, this is basically rote as it's just filling it a bunch of missing functions.
Note that
getfenvandsetfenvremain unimplemented, as they're Lua 5.1-isms that would be very difficult to implement and are rarely used. Users can adapt their scripts to useloadinstead as a workaround.LuaAllowedFunctionsThis controls the set of global functions available to a Lua script, including functions grouped under
redis.This is mostly a defense in depth thing for when Garnet is hosted as a service, allowing problematic functions to be disabled without code changes.
cjsonSerialization is bespoke because of the need to handle data passed on the Lua stack.
Deserialization uses .NET's built-in
JsonNode.cmsgpackRather than pull in a dependency, I implemented both serialization and deserialization.
The format isn't too complicated and serialization would be bespoke for the same reasons as
cjson.encode, so it seemed like a reasonable decisions.Linux & .NET 9+ test skips
Some of these tests are finally reproducing the
longjmpissue the .NET folks warned about, and failing pretty reliably. For affected tests, I'm skipping the exception parts on with a TODO for the next phase.Benchmarks
mainis as of225b2ae13c37e9583b752e068a4058b603f69c0aluaRuntimeLibrariesis as of225b2ae13c37e9583b752e068a4058b603f69c0aJust reporting
LuaRunnerOperations(and just Native memory management) as that's where the impact is expected to be largest.TL;DR - Construction costs are up, as expected, since the loader block is much larger now. Biggest relative hit is in
ConstructSmallwhere the loader block dominates, and is ~34%. Compilation is somewhat improved (~20%), as more of the loader runs at construction time.I can picture a way to speed this up (basically, keeping some
LuaStateWrappers pre-initialized and having sessions obtain them on demand), but I'm not sure it's worth the complexity.mainluaRuntimeLibrariesRelated Issues