Skip to content

Reject untrusted Lua bytecode while preserving script caching - #2138

Open
Tiago Nápoli (tiagonapoli) wants to merge 24 commits into
microsoft:mainfrom
tiagonapoli:tiagonapoli/fix-lua-bytecode
Open

Reject untrusted Lua bytecode while preserving script caching#2138
Tiago Nápoli (tiagonapoli) wants to merge 24 commits into
microsoft:mainfrom
tiagonapoli:tiagonapoli/fix-lua-bytecode

Conversation

@tiagonapoli

@tiagonapoli Tiago Nápoli (tiagonapoli) commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • reject customer-supplied Lua binary chunks for EVAL and SCRIPT LOAD
  • compile customer input using Lua text-only mode before generating cacheable bytecode
  • preserve Garnet's application bytecode cache and binary-only session loading performance
  • tag script data as source text or Garnet-generated bytecode so only internally generated chunks reach the binary loader
  • remove the fallback that returned original customer bytes when compilation failed
  • use exact-length text loading for internal loadstring compatibility handling

Security boundary

Customer input is always treated as source text and compiled with mode t. Only successful internal compilation produces a GarnetGeneratedBinary chunk, which is cached and subsequently loaded with mode b. The public LuaScriptHandle constructor creates source-text handles, so arbitrary host-provided bytes cannot opt into binary loading.

Testing

  • full Lua scripting suite on .NET 8: 638 passed, 30 skipped, 0 failed
  • full Lua scripting suite on .NET 10: 638 passed, 30 skipped, 0 failed
  • full Debug solution build for net8.0 and net10.0: 0 warnings, 0 errors
  • regression coverage for binary-safe EVAL, SCRIPT LOAD, cache exclusion, blocked side effects, compilation failure output, embedded NUL input, host-inserted source, and cross-session caching

Supersedes #2111 while retaining bytecode caching.

Tiago Napoli added 9 commits September 15, 2026 14:34
Tiago Napoli
Reject untrusted Lua bytecode
77ea719 
Keep application bytecode caching while requiring customer input to compile in text-only mode and tagging internally generated chunks for binary-only loading.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Clarify Lua bytecode regression tests
91d6652 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Clarify Lua compilation boundary test
74078ba 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Use neutral Lua input terminology
2508af6 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Simplify Lua bytecode regression setup
fde3ed9 
Generate the test bytecode directly through Garnet's internal compiler instead of round-tripping through a Lua script.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Assert rejected Lua bytecode has no side effect
adfe785 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Centralize persistent script hash allocation
ae90c7f 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Remove internal script cache test
57403d8 
Keep regression coverage focused on supported network-facing scripting workflows.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Keep script hash handling unchanged
5e15493 
Remove the unrelated ScriptHashKey allocation refactor from the Lua bytecode fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
@tiagonapoli
Tiago Nápoli (tiagonapoli) marked this pull request as ready for review September 16, 2026 17:21
Copilot AI balanced review requested due to automatic review settings September 16, 2026 17:21
Tiago Napoli
Make Lua source handle creation explicit
baefea5 
Replace the implicit source-data constructor with a named FromSource factory.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Copilot AI reviewed Sep 16, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The claimed host-inserted source-cache behavior lacks automated regression coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens Lua scripting by treating customer input as text while retaining Garnet-generated bytecode caching.

Changes:

  • Adds explicit text and trusted-binary chunk types and loading modes.
  • Compiles source safely before caching internal bytecode.
  • Adds bytecode rejection and exact-length parsing tests.
File summaries
File Description
LuaScriptTests.cs Adds scripting security regressions.
LuaScriptRunnerTests.cs Tests compilation and chunk tagging.
SessionScriptCache.cs Separates source compilation from cached loading.
NativeMethods.cs Adds text-only and binary-only native loaders.
LuaStateWrapper.cs Exposes mode-specific loading wrappers.
LuaScriptHandle.cs Tags cached script data by origin.
LuaRunner.Loader.cs Safely compiles source into bytecode.
LuaRunner.Functions.cs Propagates chunk loading mode.
LuaRunner.cs Distinguishes public source from internal bytecode.
LuaCommands.cs Routes commands through secure loading paths.
LuaScriptCacheOperations.cs Updates cache benchmarks for the new APIs.
Review details
  • Files reviewed: 11/11 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread libs/server/Lua/SessionScriptCache.cs Outdated
Comment on lines +210 to +213
if (luaScriptHandle.Chunk.Kind == LuaScriptChunkKind.GarnetGeneratedBinary)
return TryLoadCompiled(session, luaScriptHandle.Chunk, digest, ref luaScriptHandle, out runner, out digestOnHeap);

return TryCompileAndLoad(session, luaScriptHandle.ScriptData.Span, digest, ref luaScriptHandle, out runner, out digestOnHeap);
Tiago Napoli added 13 commits September 16, 2026 10:37
Tiago Napoli
Simplify Lua script cache branches
9120748 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Restore LuaRunner constructor formatting
8e5b33f 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Finish Lua formatting cleanup
c28562a 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Clarify Lua source factory name
4e7c1d2 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Remove redundant Lua loader comments
49cbfbc 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Clarify Lua script cache control flow
1291fa8 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Separate Lua source and cache loading
15efd15 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Centralize Lua script loading
fa64aaf 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Simplify LuaRunner chunk construction
9bde2d9 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Clarify Lua load failure handling
0d184d1 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Format Lua load failure comment
27ccbd5 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Clarify Lua runner cache naming
a3dd5aa 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Tiago Napoli
Deduplicate Lua out-of-memory responses
e56277f 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ec2a6abf-4bb4-48e1-a752-32672cb07303
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tiagonapoli