Oberon00 / luabind

Luabind is a library that helps you create bindings between C++ and Lua.
http://oberon00.github.io/luabind/
Other
46 stars 13 forks source link

Fixes alignment assumption in `object_rep` placement new #50

Closed kaetemi closed 2 years ago

kaetemi commented 2 years ago

The object_rep class is allocated and initialized as follows:

void* storage = lua_newuserdata(L, sizeof(object_rep));
object_rep* result = new (storage) object_rep(0, cls);

The class also contains the following member:

boost::aligned_storage<instance_buffer_size> m_instance_buffer;

The alignment guarantee of lua_newuserdata can be found from:

struct cD { char c; union { LUAI_MAXALIGN; } u; };
const int maxalign = offsetof(struct cD, u);

This is likely 8 bytes.

The alignment of boost::aligned_storage can be found from alignof(std::max_align_t).

This may be either 8 or 16 bytes, depending on the platform.

When the class is aligned to 16 bytes, movaps may be used by the compiler, as the compiler assumes the allocation has the specified alignment.

But when the allocation is only aligned to 8 bytes at the same time, this causes an alignment crash in the constructor called during placement new.

Patch applies the correct alignment guarantee of lua_newuserdata to the storage.

Oberon00 commented 2 years ago

Thanks, this looks like it could be the actual issue behind #44 indeed. Very subtle, nice detective work! This looks like a low-risk change to me and solves a reported issue, so I'll merge it without verifying manually myself.

CC @timblechmann

kaetemi commented 2 years ago

Now the remaining question is, can whatever that ends up stored inside this object_rep allocation be an object that has higher alignment requirements? If so, then void* allocate(std::size_t size) in object_rep needs to be changed to include the alignment, and any higher alignment requirements can just be aligned at runtime.

Oberon00 commented 2 years ago

That might in theory be a problem I think. But since lua_newuserdata should have the same issue, it is hopefully not relevant in practice. It's been a long time since I looked at the code, but maybe due to the now correct usage of aligned_storage, the compiler would even be able to detect any underalignment and it would just be a performance issue. But not sure about that.