fix(lua): globals protection, stack leak, and return/arg compatibility#5
Open
fatal10110 wants to merge 4 commits into
Open
fix(lua): globals protection, stack leak, and return/arg compatibility#5fatal10110 wants to merge 4 commits into
fatal10110 wants to merge 4 commits into
Conversation
Two related sandbox bugs in the Lua runtime:
1. luaopen_base (Lua 5.1) returns two tables (the globals table and the
coroutine table), but open_allowed_libs popped only one. The leaked
globals table sat on the stack and was treated as the script's return
value, so any script with no explicit `return` (e.g. `eval "print('a')" 0`)
replied with an empty array instead of OK. Clear the stack after opening
the allowed libraries.
2. The sandbox loaded the base library wholesale and never installed Redis's
globals protection, so `print` ran and undeclared-global access silently
returned nil. Install a protected metatable on the globals table whose
__index / __newindex raise Redis's exact errors, remove the base-lib
globals real Redis does not expose (print, loadstring, collectgarbage,
gcinfo, newproxy, coroutine), and assign KEYS/ARGV via raw set so they
bypass the protection.
`eval "print('a')" 0` now errors with
"Script attempted to access nonexistent global variable 'print'", matching
real Redis. Adds tests for both the nonexistent-global read and the
create-global write.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up compatibility fixes found while reviewing the Lua runtime against a real redis-server 7.2: - No-return scripts now reply with nil instead of OK (matches real Redis). - Lua number return values are truncated to integer replies (`return 3.7` -> 3) rather than encoded as bulk strings. - A returned table carrying both `ok` and `err` is treated as an error (`err` wins), matching Redis's field precedence. - Array replies stop at the first nil instead of relying on the Lua length operator's undefined border for tables with holes. - redis.call/pcall reject boolean (and other non-string/number) arguments with "Lua redis lib command arguments must be strings or integers", raised without a script-position prefix, exactly like real Redis. - Writing a global now raises "Attempt to modify a readonly table" (the Redis 7 wording) instead of the older "create global variable" message. - coroutine is left available; real Redis exposes it to scripts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fatal10110
changed the title
Make the engine hand the host everything it needs to render Redis errors,
so the host never has to infer wording or codes from message strings.
- Globals-write protection now emits a coded marker carrying the variable
name; the TS layer renders the default (Redis 7) "Attempt to modify a
readonly table" message and attaches `meta` ({ kind, name, line, sha }) so
a host can re-render the pre-7.0 "create global variable '<name>'" wording
for older compatibility profiles. The sha is the one the engine already
computed, so the host does not recompute it.
- Script-aborting errors carry an explicit error code: a propagated command
code (e.g. WRONGTYPE) is preserved, otherwise the default "ERR" is set.
Hosts can now emit the code verbatim instead of defaulting it themselves.
- redis.error_reply() prepends the default "ERR " code only when the message
has no code of its own (no leading space), matching real Redis; returned
error tables ({ err = ... }) stay verbatim.
ReplyValue's error member gains an optional `meta: ReplyErrorMeta`.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fatal10110
force-pushed
the
fix/lua-globals-protection
branch
from
ab22acb to
665a53e
Compare
The engine no longer composes Redis's user-facing error wording. It
classifies engine-originated errors (globals protection) with an opaque
machine `kind` and lets the host render.
- runtime.c: emit symmetric coded markers for both globals-protection
errors (`__RLUA_E__:global-read:<name>` and `:global-write:<name>`);
global-read no longer hardcodes English.
- engine.ts: parse the marker generically (kind = second segment, opaque
passthrough); error reply carries { kind, name, line, sha } in meta and
the bare kind in err. Lua/redis.call errors pass through with their own
message + code. Remove the renderRedisError helper (host renders).
- types.ts: ReplyErrorMeta.kind is now an open string; document known
kind -> wording mappings for host authors.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Reviewed the Lua runtime against a real redis-server 7.2 and fixed every divergence found. Original trigger:
eval "print('a')" 0returned(empty array)against the mock; real Redis errors.Bugs fixed
Sandbox / globals
luaopen_base(Lua 5.1) pushes two tables (globals + coroutine);open_allowed_libspopped only one. The leaked globals table became the script's "return value", so any no-return script replied with an empty array. Now the stack is cleared after opening the allowed libs.Script attempted to access nonexistent global variable 'NAME'; writes raiseAttempt to modify a readonly table(Redis 7 wording). Removed base-lib globals real Redis hides (print,loadstring,collectgarbage,gcinfo,newproxy).coroutineis kept — real Redis exposes it.KEYS/ARGVare assigned via raw set so they bypass the protection.Return-value / argument conversion
OK.return 3.7→3) instead of bulk strings.okanderris an error (errwins), matching Redis field precedence.redis.call/pcallreject boolean (and other non-string/number) args withLua redis lib command arguments must be strings or integers, raised without a script-position prefix — exactly like real Redis.Verified against real Redis 7.2
print('a'),x=5,return 3.7,return true/false,{ok=,err=},{1,2,nil,4},{err=}, no-return,type(coroutine), boolean arg — all byte-for-byte matching (error wording + script SHA included).Tests
Added coverage for nonexistent-global read, readonly-write, float truncation, no-return nil,
ok/errprecedence, coroutine availability, and boolean-arg rejection. Full suite: 147/147 green.Known gaps (follow-ups, not in this PR)
redis.replicate_commands(),redis.set_repl, and theredis.REPL_*constants are not implemented (real Redis has them;replicate_commands()is a no-op returning true in 7.x).redis.error_reply('foo')returnsfoo; real Redis prepends a defaultERRcode when the message has no error-code prefix.🤖 Generated with Claude Code