satoren / kaguya

C++ binding to Lua
Boost Software License 1.0
345 stars 70 forks source link

segfault on assignment of unknown/unregistered property #72

Closed iar74 closed 7 years ago

iar74 commented 7 years ago

Situation:

class A { public: A(){} int a; }; L["A"].setClass( kaguya::UserdataMetatable\<A\>() .setConstructors<A()>() .addProperty("a", &A::a )); L("a = A.new() a.b=5")

The assignment of an unknown property crashes in:

inline int property_newindex_function(lua_State *L) { <**snip/**> lua_pushvalue(L, key); lua_pushvalue(L, value); lua_rawset(L, table); // <- here ( or to be more precise, within lua_rawset) return 0; }

I compiled this on

All with c++11 enabled and with kaguya-master, as of Aug., 10th 2017 (should be current), and lua 5.3.4.

I would expect a lua error (assignment of non-existent property), or an enrichment (a.b->5), not a segfault. Am I doing something wrong?

Any hint appreciated.

Thanks for the good work!

iar74 commented 7 years ago

I have dug a little deeper. The problem is that in case of a non-existant property, the property_newindex_function tries to rawset a uservalue, which is illegal. I added a test for nil type on access to the property and bailed out with luaL_error. This behaves as I would expect it, though not sure what the intent was. Please refer to 84acfd3d6416a660ff1b5175f8cad65da2219b7f.

satoren commented 7 years ago

Thank you for your report. I think better to return nil if getting unregistered property.

iar74 commented 7 years ago

Yes, returning nil in this case surely is an option. Though sometimes, a kind of symmetry might be what is desired (one cannot add properties to the uservalue <-> one can not get unregistered properties from the uservalue). Would you be open to have a compile time flag that decides whether the property get is strict (error on access to unregistered property) or more lua-like (return nil on access to unknown property)? I made a sketch implementation here: 2654c0cc259f0bfb5ef5504a6241b90f50dedf2a.

Please let me know your thoughts on this.

iar74 commented 7 years ago

Thanks for adding the change #72.