Skip to content

Fix the error-handler stack index in xpcall - #147

Open
PerryLink wants to merge 1 commit into
Shopify:mainfrom
PerryLink:fix/xpcall-handler-index
Open

PerryLink wants to merge 1 commit into
Shopify:mainfrom
PerryLink:fix/xpcall-handler-index

Conversation

@PerryLink

Copy link
Copy Markdown

What

xpcall with an error handler can hang or report the wrong error, because the handler is stored as a
frame-relative stack index and then read as an absolute one:

// lua.go:400, ProtectedCallWithContinuation
l.errorFunction = l.AbsIndex(-1)          // stored relative to the current frame
...
// debug.go:123, errorMessage
l.stack[l.errorFunction]                  // read as an absolute index

When the call happens inside a running chunk (callInfo.function != 0), the two disagree: the handler
is looked up at the wrong stack slot. Minimal reproduction:

local ok, e = xpcall(function() error("x") end, function(m) return "H:" .. m end)
return ok, e
  • before: the call does not terminate (panic: test timed out after 10s — unbounded recursion
    re-entering the outermost chunk), or, with a test that pre-fills the slot, fails with
    handler result: error in error handling;
  • after: returns false, "H:...:1: x".

The change

Store the handler as an absolute index by adding the current frame base:

l.errorFunction = l.callInfo.function + l.AbsIndex(-1)

One line. pcall (errorFunction == 0) and the top-level host ProtectedCall
(callInfo.function == 0) are unaffected by construction.

Verification

go test -vet=off -run TestXPCall -v -count=1:

  • before (pristine main + only the new test): FAIL with handler result: error in error handling;
  • after (git apply patch.diff, apply --check clean): both tests pass;
  • full suite go test -vet=off ./...: ok.

The regression test pre-fills the stale slot with a non-callable value (with a comment explaining why)
so the wrong index fails fast instead of hanging for minutes.

Dedupe: searched xpcall / error handler / hang / recursion on the repo's issues and PRs — no match.

Shopify CLA has been signed.

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.

1 participant