kikito / inspect.lua

Human-readable representation of Lua tables
https://github.com/kikito/inspect.lua
MIT License
1.38k stars 196 forks source link

Lua crashes when inspecting _G #62

Closed Mellnik closed 1 year ago

Mellnik commented 1 year ago

Environment

The Crash When inspecting _G it seems like the metamethod __eq is called on the ExampleClass. But apparently there is no valid userdata passed resulting in a crash.

class ExampleClass
{
public:
    ExampleClass() :
        a(0), b(0), c(0)
    {

    }

    string tostring() const
    {
        return "whatever";
    }

    bool operator==(const ExampleClass& v) const
    {
        return a == v.a && b == v.b && c == v.c;
    }

    int a, b, c;
};

lua_State* L = luaL_newstate();

luaL_openlibs(L);

luabridge::getGlobalNamespace(L)
    .beginClass<ExampleClass>("ExampleClass")
        .addConstructor<void(*) ()>()
        .addFunction("tostring", &ExampleClass::tostring)
        .addFunction("__tostring", &ExampleClass::tostring)
        .addFunction("__eq", &ExampleClass::operator==)
    .endClass()
;

luaL_dostring(L, "inspect = require('inspect'); print(inspect(_G));");
main.exe!luabridge::detail::Userdata::getPointer() Line 46  C++
main.exe!luabridge::detail::Userdata::get<ExampleClass>(lua_State * L, int index, bool canBeConst) Line 254 C++
main.exe!luabridge::detail::CFunc::CallConstMember<bool (__cdecl ExampleClass::*)(ExampleClass const &)const>::f(lua_State * L) Line 291    C++
main.exe!precallC(lua_State * L, StackValue * func, int nresults, int(*)(lua_State *) f) Line 506   C
main.exe!luaD_precall(lua_State * L, StackValue * func, int nresults) Line 570  C
main.exe!ccall(lua_State * L, StackValue * func, int nResults, int inc) Line 607    C
main.exe!luaD_call(lua_State * L, StackValue * func, int nResults) Line 620 C
main.exe!luaT_callTMres(lua_State * L, const TValue * f, const TValue * p1, const TValue * p2, StackValue * res) Line 129   C
main.exe!luaV_equalobj(lua_State * L, const TValue * t1, const TValue * t2) Line 612    C
main.exe!luaV_execute(lua_State * L, CallInfo * ci) Line 1566   C
main.exe!ccall(lua_State * L, StackValue * func, int nResults, int inc) Line 611    C
main.exe!luaD_callnoyield(lua_State * L, StackValue * func, int nResults) Line 628  C
main.exe!f_call(lua_State * L, void * ud) Line 1042 C
main.exe!luaD_rawrunprotected(lua_State * L, void(*)(lua_State *, void *) f, void * ud) Line 147    C
main.exe!luaD_pcall(lua_State * L, void(*)(lua_State *, void *) func, void * u, __int64 old_top, __int64 ef) Line 926   C
main.exe!lua_pcallk(lua_State * L, int nargs, int nresults, int errfunc, __int64 ctx, int(*)(lua_State *, int, __int64) k) Line 1067    C
main.exe!main() Line 99 C++
kikito commented 1 year ago

Hello, thank you for reporting this.

I am not familiar with LuaBridge and replicating this issue would require for me to get familiar with it first.

I have an inkling of what might be going on here.

In general, inspect should just output <userdata x> where x is the order in which inspect has encountered said userdata while parsing its input.

The "has encountered said userdata" test might be where inspect is trying to invoke __eq on your userdata. My assumption was that every single Lua value is comparable with other values - if anything else, this is needed because in Lua any value can be nil, so you at least need to be able to nil-check.

If this is correct, then the possibilities I see are:

As I mentioned at the beginning I am not familiar with LuaBridge and I'm afraid I don't have the bandwidth to investigate this. Please feel free to send a Pull Request.

Mellnik commented 1 year ago

Hi kikito,

thank you very much for your fast reply and feedback.

I think that in no case Lua scripts should be able to crash the host. Of course this only applies if "safe" methods are exposed to Lua and the user C side (host) properly interacts with Lua. This includes properly exposing userdata, as you said, and running code in protected mode (pcall).

From what I see inspect only uses getmetatable and setmetatable which I consider safe as long as the host took preventive measures.

By digging into LuaBridge I found out that it sets the metatable field with a "nil". (See here) Effectively deleting that key. According to Lua documentation metatable must be set in order to prevent any tempering with the default set/getmetatable (not the debug one).

I have modified that part by replacing lua_pushnil with lua_pushboolean.

Now inspect no longer crashes because the metatable of their userdata is no longer accessible.

The LuaBridge documentation states that they set a boolean to __metatable but apparently the current source does not do that.

I am unsure if this fix is the actual proper way of doing it. Or if there is another issue with LuaBridge.

Anyway I am going to report that to LuaBridge.

I think you can close this issue if you have no more to add.

Thank you for your time!

Mellnik commented 1 year ago

Not really related to the before issue but I just saw that inspect also uses rawget. Would it be possible to add a check whether rawget is available because in some environments this function is not exposed. inspect can still deliver plenty information.

kikito commented 1 year ago

That is easier to model. Do you mind giving this a look, see if it works for you?

https://github.com/kikito/inspect.lua/tree/feat-optional-rawget

Mellnik commented 1 year ago

Thanks. For me it works well and prints the same output as with rawget.

kikito commented 1 year ago

alright, that change is merged in master now.

I'll close this since LuaBridge seemed to have an issue after all, please reopen if it turns out I was wrong.