Skip to content

compile: fix panic and silent corruption when assigning to a field of a non-table value#538

Open
snowyukitty wants to merge 1 commit into
yuin:masterfrom
snowyukitty:fix/index-nontable-assign-panic
Open

compile: fix panic and silent corruption when assigning to a field of a non-table value#538
snowyukitty wants to merge 1 commit into
yuin:masterfrom
snowyukitty:fix/index-nontable-assign-panic

Conversation

@snowyukitty

Copy link
Copy Markdown

Fixes #524 .

Problem

Assigning to a field or index of a non-table value can crash the VM with a Go
nil pointer dereference, instead of raising the usual attempt to index error:

debug.setmetatable("string", {})
("Lua").key = 1
-- panic: runtime error: invalid memory address or nil pointer dereference
--   ... gopher-lua/state.go: (*LState).metatable ...

Depending on the surrounding register layout the same class of statement can also
silently corrupt a nearby value instead of erroring:

local t = {}
(5).field = 1   -- no error; writes t.field = 1

Reading the same expression is fine (return ("Lua").key raises the proper
error), so only the write path is affected.

Cause

The object of an assignment target (t in t.k = v) is compiled by
compileAssignStmtLeft with compileExprWithKMVPropagation. For a constant
object it folds the OP_LOADK into an RK (constant) operand. But that object
is operand A of OP_SETTABLE/OP_SETTABLEKS, which the VM always reads as a
register: R(A)[RK(B)] := RK(C). Operand A is only 8 bits, so the folded
constant index aliases an unrelated register — usually a silent wrong write, or a
nil dereference in metatable() when that slot is unset.

The read path (AttrGetExpr in compileExpr) already uses MV propagation, which
is why reads work.

Fix

Compile the assignment-target object with compileExprWithMVPropagation, so it is
always materialised into a register — exactly like the read path. MV never folds
OP_LOADK; keys and values keep using RK, which operands B and C accept. It is a
one-token change, after which the existing attempt to index a non-table object(<type>) error is raised, with the correct type.

Tests

Added an issue #524 block to _glua-tests/issues.lua covering the silent
corruption, the reported panic, the corrected value type in the message, and that
a valid (t).x = 1 still works. go test ./... stays green, and the regression
fails on the current base.

… a non-table value

The object of an assignment target (the `t` in `t.k = v`) becomes operand A of
OP_SETTABLE/OP_SETTABLEKS, which the VM always reads as a register R(A). But
compileAssignStmtLeft compiled that object with KMV propagation, which folds a
constant object such as ("s").k = v or (5).x = v into an RK (constant) operand.
Its truncated index then aliases an unrelated register, so the assignment either
silently wrote to the wrong value or, when that register was unset, panicked with
a nil pointer dereference (e.g. after debug.setmetatable("string", {})).

Compile the object with MV propagation instead, matching the read path, so
operand A is always a materialised register and the existing "attempt to index a
non-table object" error is raised with the correct type.

Fixes yuin#524
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic on attempt to index a string value

1 participant