Lua GC crash

[nimble0]

I've had trouble getting easy reproduction of this but maybe what I've worked out will help:

• EventDispatcher::DispatchEvent creates an event (with C++ new).
• The event gets sent to Lua with LuaType::push, marked not to delete when GCed.
• The event is deleted on the C++ side.
• A new object which is deleted when GCed (e.g. ElementStyleProxy) is created at the same address as the event.
• The new object gets sent to Lua with LuaType::push, marked to delete when GCed, overwriting the original mark not to delete when GCed.
• The new object gets GCed calling LuaType::gc_T, which deletes the object in C++.
• The event gets GCed calling LuaType::gc_T, which deletes the same address in C++ causing a segfault.

[mikke89]

So if I understand correctly, everything sounds fine here until the last point. The bug is that the event gets garbage collected even though the argument is false on push? And this only happens if by pure random chance the event shares address with another later created and GCed object?

[nimble0]

Yes, that's correct. Although I might be more inclined to attribute the problem to Lua holding references to deleted objects. I imagine there might be some strange behaviour if an event object reference was kept and later accessed (in Lua) as well.

[mikke89]

So I had another look at this. I'm no Lua wizard, but I wonder if we can solve this simply by setting the lua table value to 1 (= do not call delete) after the line where the object is garbage collected and deleted?

Since it's impossible in C++ that two live objects share the same address, the next time garbage collection is called on the same pointer it must already have been destroyed in C++. However, one assumption here is that Lua calls gc_T in reverse order of construction. Do you know whether this is true? Eg. in you example it would mean that gc_T("new object") is always called before gc_T(event).

Can you test the commit with the proposed fix? I can't really reproduce the issue, so I will need some help to determine if this really works at all. If you know Lua well, I would appreciate any feedback on my code as well, thanks!

[nimble0]

The stack index in that commit should be -3 instead of -2.

However, one assumption here is that Lua calls gc_T in reverse order of construction. Do you know whether this is true?

I think that is the case. "... the finalizers for objects are called in the reverse order that they were marked for collection, among those collected in that cycle ..." (https://www.lua.org/manual/5.2/manual.html#2.5.1)

However, this would still leave references to freed memory hanging around in Lua. I think bugs relating directly to this might be easier to reproduce, I tried holding onto an Event in Lua and crashed immediately in a quick test.

[mikke89]

Thanks, I changed the stack index now.

However, this would still leave references to freed memory hanging around in Lua. I think bugs relating directly to this might be easier to reproduce, I tried holding onto an Event in Lua and crashed immediately in a quick test.

I'd consider that a related, but separate issue. To me it doesn't really make sense to hold onto events, what kind of use cases are we talking about here?

[nimble0]

It's not use cases I'm thinking of. I think Lua should be sandboxed for safety and security.

[mikke89]

The library is pretty much built with the presumption of using known and trusted sources. Other cases have never been the goal and it would take a monumental effort to harden the library for such use cases at this point.

[nimble0]

I thought that was the case but I still think it's preferable to try and eliminate vulnerabilities when discovered even if there's no coordinated effort to try to remove all of them. And the safety aspect does apply even when the data is trusted, it's not expected to encounter memory bugs when using a garbage collected language.

[mikke89]

Yeah, that is fair and I generally agree. I'm not the most experienced with Lua but I will certainly review and accept pull requests if you or anyone else have ideas to solve such issues. Meanwhile, I consider the original issue fixed now.