Fix Lua object garbage collection bug #340
Conversation
As discovered by Ishq, multiple extant userdata Lua objects sometimes had the same underlying 'obj' pointer (pointer to the underlying object which is used as the key in the "DO NOT TRASH" table). Which led to crashes. Lua does not immediately garbage-collect dead objects. This means that for the non-owning objects (gc=false), another object could be potentially be created at the same address before garbage collection occurs, thereby clobbering the DO NOT TRASH entry. The fix: use the address of the userdata, which is guaranteed to be unique, as the DO NOT TRASH key.
|
Thanks for upstreaming this fix! This reminds me a lot about the discussion here: #216. If not the same issue, then at least very similar. What do you think about the proposed (and merged) solution there-in? Do you think that solution already fixes the issue you have? And if not, could you comment on whether there could be any conflict between these two fixes. I did run and do some basic testing with this change, and everything seems to work fine, however these issues can be hard to reproduce so it would be nice to hear your thoughts. |
|
We did cherry pick that change prior to this one and we could still reproduce the crash. Maybe our platform (Google's NaCl runtime) is more prone to the aggressive pointer location reuse but we had a 100% repro that we can use to verify fixes if you'd like 😁. That fix is insufficient because it only fixes the case where the pointer is reused once before the original non gc object is gc'd. But if it's reused twice and the original object is gc'd before the 2nd object is sets the DNT table to true, we would still crash |
|
I confirm I reproduce the crash on current RmlUi master but I don't manage to reproduce the bug once I cherry-picked this commit on current RmlUi master. I used @DolceTriade's branch to port Unvanquished to latest RmlUi version to reproduce the crash on master and test the fix on master: |
|
Thanks guys! Sounds good to me, that is more than enough to justify this change. Appreciate the pull request. |
Disclaimer: this fix has only been tested with our 2-year-old version of the library.