ThePhD / sol2

Sol3 (sol2 v3.0) - a C++ <-> Lua API wrapper with advanced features and top notch performance - is here, and it's great! Documentation:
http://sol2.rtfd.io/
MIT License
4.2k stars 516 forks source link

Registering New Usertype Can Replace Existing Table / Object #1397

Open atom0s opened 2 years ago

atom0s commented 2 years ago

sol version: https://github.com/ThePhD/sol2/commit/4de99c5b41b64b7e654bf8e48b177e8414a756b7 Lua version: MoonJit via https://github.com/AshitaXI/moonjit/commit/094d971c0a9f76025e2c61fab65969aea7fdb753 Compiler version: VS2022 17.3.2 (Using C++23 /std:c++latest x86)

Issue Description:

When registering a new usertype, it is possible to overwrite the existence of an existing variable/object/table in the state without realizing/knowing. If the user creates a named table and then later registers a new usertype with the same name, the previously created table will be nuked, and vise versa if done in reverse.

For example:

    {
        auto u_foo = lua.new_usertype<some::inner::name::foo>("foo", sol::no_constructor);
        u_foo.set("i", sol::property(&some::inner::name::foo::get_int_val));
        u_foo.set("print", &some::inner::name::foo::print);
    }
    {
        auto foo        = lua.create_named_table("foo");
        foo["instance"] = f.get();
    }

or

    {
        auto foo        = lua.create_named_table("foo");
        foo["instance"] = f.get();
    }
    {
        auto u_foo = lua.new_usertype<some::inner::name::foo>("foo", sol::no_constructor);
        u_foo.set("i", sol::property(&some::inner::name::foo::get_int_val));
        u_foo.set("print", &some::inner::name::foo::print);
    }

An example of where this can be a bigger issue is in an environment that makes heavy use of coroutines. When the state is reset or finalized, you will get an exception about the user type being destroyed incorrectly. This is what that will look like:

Exception thrown at 0x00D81866 in test.exe: 0xC0000005: Access violation reading location 0x00000078.
    test.exe!_lua_rawgeti()    Unknown Symbols loaded.
>   test.exe!sol::basic_reference<0>::deref() Line 546  C++ Symbols loaded.
    test.exe!sol::basic_reference<0>::~basic_reference<0>() Line 636    C++ Symbols loaded.
    test.exe!sol::u_detail::binding<std::string,sol::basic_reference<0>,void>::~binding<std::string,sol::basic_reference<0>,void>() C++ Non-user code. Symbols loaded.
    test.exe!sol::u_detail::binding<std::string,sol::basic_reference<0>,void>::`scalar deleting destructor'(unsigned int)   C++ Non-user code. Symbols loaded.
    test.exe!std::default_delete<sol::u_detail::binding_base>::operator()(sol::u_detail::binding_base * _Ptr=0x0077ca50) Line 3141  C++ Non-user code. Symbols loaded.
    test.exe!std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>::~unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>() Line 3254   C++ Non-user code. Symbols loaded.
    test.exe!std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>::`scalar deleting destructor'(unsigned int)  C++ Non-user code. Symbols loaded.
    test.exe!std::destroy_at<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>>(std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>> * const _Location=0x007774a4) Line 315    C++ Non-user code. Symbols loaded.
    test.exe!std::_Default_allocator_traits<std::allocator<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>>>::destroy<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>>(std::allocator<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>> & __formal={...}, std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>> * const _Ptr=0x007774a4) Line 689  C++ Non-user code. Symbols loaded.
    test.exe!std::_Destroy_range<std::allocator<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>>>(std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>> * _First=0x007774a4, std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>> * const _Last=0x007774b8, std::allocator<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>> & _Al={...}) Line 949  C++ Non-user code. Symbols loaded.
    test.exe!std::vector<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>,std::allocator<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>>>::_Tidy() Line 1907    C++ Non-user code. Symbols loaded.
    test.exe!std::vector<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>,std::allocator<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>>>::~vector<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>,std::allocator<std::unique_ptr<sol::u_detail::binding_base,std::default_delete<sol::u_detail::binding_base>>>>() Line 809  C++ Non-user code. Symbols loaded.
    test.exe!sol::u_detail::usertype_storage_base::~usertype_storage_base() Line 614    C++ Symbols loaded.
    test.exe!sol::u_detail::usertype_storage<exampletype>::~usertype_storage<exampletype>() C++ Non-user code. Symbols loaded.
    test.exe!sol::u_detail::usertype_storage<exampletype>::`scalar deleting destructor'(unsigned int)   C++ Non-user code. Symbols loaded.
    test.exe!std::destroy_at<sol::u_detail::usertype_storage<exampletype>>(sol::u_detail::usertype_storage<exampletype> * const _Location=0x005ea438) Line 315  C++ Non-user code. Symbols loaded.
    test.exe!std::_Default_allocator_traits<std::allocator<sol::u_detail::usertype_storage<exampletype>>>::destroy<sol::u_detail::usertype_storage<exampletype>>(std::allocator<sol::u_detail::usertype_storage<exampletype>> & __formal={...}, sol::u_detail::usertype_storage<exampletype> * const _Ptr=0x005ea438) Line 689  C++ Non-user code. Symbols loaded.
    test.exe!sol::detail::user_alloc_destroy<sol::u_detail::usertype_storage<exampletype>>(lua_State * L=0x005e01c0) Line 512   C++ Symbols loaded.
    test.exe!sol::u_detail::destroy_usertype_storage<exampletype>(lua_State * L=0x005e01c0) Line 826    C++ Symbols loaded.
    test.exe!_lj_BC_FUNCC()    Unknown Non-user code. Symbols loaded.
    00010660()  Unknown Non-user code

This can ultimately lead to long debugging sessions/headaches trying to track down what the cause is, because this is the same exact exception stack/output you'll get from an unfree'd object dangling in the state when its reset/finalized. (ie. if you have a vector of objects from the Lua state and reset the state before clearing the vector.)

This issue is obviously expected behavior for Lua, so my report is not to expect a fix to allow the above code examples to work. (I don't think there is honestly a way to make those work as-is without adding a large amount of overhead/performance hits to sol which would not be ideal. This would basically require adding name mangling to the registered user-type global table that a script would want to access, then also adding demangling on all lookups to see if a usertype is being used. This is obviously not ideal.)

Instead, a potential idea that could be enabled with a new #define would be adding a check in sol to see if an existing type was already registered with a given name when calling things like new_usertype and create_named_table. This will at least help deal with the C++ side of things which can help reduce the issue in a debugging setup where the user may have a bit of sol's heavier defines enabled. This can then be disabled to remove any performance hit when running in a live/release mode build.