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
Open
compile: fix panic and silent corruption when assigning to a field of a non-table value#538snowyukitty wants to merge 1 commit into
snowyukitty wants to merge 1 commit into
Conversation
… 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
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
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.
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 indexerror:Depending on the surrounding register layout the same class of statement can also
silently corrupt a nearby value instead of erroring:
Reading the same expression is fine (
return ("Lua").keyraises the propererror), so only the write path is affected.
Cause
The object of an assignment target (
tint.k = v) is compiled bycompileAssignStmtLeftwithcompileExprWithKMVPropagation. For a constantobject it folds the
OP_LOADKinto anRK(constant) operand. But that objectis operand A of
OP_SETTABLE/OP_SETTABLEKS, which the VM always reads as aregister:
R(A)[RK(B)] := RK(C). Operand A is only 8 bits, so the foldedconstant index aliases an unrelated register — usually a silent wrong write, or a
nil dereference in
metatable()when that slot is unset.The read path (
AttrGetExprincompileExpr) already uses MV propagation, whichis why reads work.
Fix
Compile the assignment-target object with
compileExprWithMVPropagation, so it isalways materialised into a register — exactly like the read path.
MVnever foldsOP_LOADK; keys and values keep usingRK, which operands B and C accept. It is aone-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 #524block to_glua-tests/issues.luacovering the silentcorruption, the reported panic, the corrected value type in the message, and that
a valid
(t).x = 1still works.go test ./...stays green, and the regressionfails on the current base.