Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix VM inconsistency caused by userdata C TM fast paths #497

Merged
merged 3 commits into from
May 24, 2022

Conversation

axstin
Copy link
Contributor

@axstin axstin commented May 23, 2022

This patch fixes differing behavior in luau_callTM, which throws a "C stack overflow" error earlier than a call to callTMres (and by extension luaD_call) would. This results in certain C metamethod calls on userdatas throwing immediately in error handlers executed after a C stack overflow:

local cso = setmetatable({}, {
	__index = function(self, i)
		return self.overflow[i]
	end
})

-- userdata with C metamethod
local userdata = newproxy(true)
getmetatable(userdata).__index = print

print(xpcall(function() return cso.overflow end, function(e)
	assert(string.find(e, "C stack overflow"))
	print("A")
	local a = userdata[123] -- OP_GETTABLEN: ok
	print("B")
	local b = userdata.StringKey -- OP_GETTABLEKS: error
	print("C")
end))
> before patch
A
userdata: 0x0000023bbbddcc38    123
B
false   error in error handling

> after patch
A
userdata: 0x000001fe6567cc38    123
B
userdata: 0x000001fe6567cc38    StringKey
C
false   nil

Lua 5.4 implements luaE_checkcstack in lstate.c. Not sure if it's better off there, in ldo.c, or whether a function should be declared at all.

Thanks.

@zeux
Copy link
Collaborator

zeux commented May 24, 2022

Thanks, this is a great find. Ordinarily I'd say we should stay closer to vanilla Lua but ldo makes more sense to me as this is where we currently have stack growth logic, and the call depth restriction is pretty artificial and doesn't actually affect thread stack management per se, just the execution semantics.

@zeux
Copy link
Collaborator

zeux commented May 24, 2022

Could you add the test that exercises this behavior to tests/conformance/errors.lua? That way we know we won't regress in the future.

@axstin
Copy link
Contributor Author

axstin commented May 24, 2022

Let me know if these tests look good.

Copy link
Collaborator

@zeux zeux left a comment

Choose a reason for hiding this comment

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

Thanks!

@zeux zeux merged commit e13f17e into luau-lang:master May 24, 2022
@axstin axstin deleted the checkcstack branch May 24, 2022 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants