Pack structs in C bindings

[vkkoskie]

This creates a separate issue out of the latter part of this comment and the one that follows.

When bindings are generated from the C headers in cryptoki-sys, they do so using whatever structure alignment and packing is assumed for the target platform unless packing is made explicit in the headers for bindgen to read. Currently, packing is only specified for Windows and left implied everywhere else.

You can confirm that forcing a 1-byte alignment for structs on other platforms that it does indeed produce bindings that differ in terms of size and field offsets, and is not the implicit default.

Meanwhile, the PKCS#11 standard (both 2.x and 3.x, Section 2.1) are very clear that

Cryptoki structures are packed to occupy as little space as is possible. Cryptoki structures SHALL be packed with 1-byte alignment.

This would seem to imply that packing be explicit for all target bindings. But when this is done, several problems arise:

1. Rust assumes a >1-byte minimum alignment for struct members, which makes referencing anything beyond the 0^th item undefined behavior. Each such instance of this (hundreds in the auto-generated tests) produces a valid, unsuppressible compiler warning. This is a [known issue]() for bindgen that doesn't appear to be nearing a solution any time soon.
2. Tests written for this crate using SoftHSM seg fault. Whether this is at the rust level (dereferencing unaligned addresses) or at the C level (mismatch with struct packing internal to SoftHSM) is unclear. The latter doesn't seem like it should be the case, but the fact that tests are currently passing would seem to be an endorsement of implicit, non-compact alignment.

So, something is incorrect here, but what exactly that is needs to be investigated. Even if it turns out the way the bindings are currently generated is correct, that fact should still be documented conspicuously to avoid further misconception.

[vkkoskie]

Well, this is frustrating.

Long story short: Seg faults I was seeing working on #66 are indeed a packing issue. It just took me a while to get around to confirming it.

I now have a clear picture of why following the spec to use 1-byte structure packing results in seg faults: libsofthsm2 doesn't follow it!

When C_GetFunctionList is called, the returned list struct is the 2-byte library version followed by all the function pointers. With default packing (on presumably all 64-bit Linux) this pads the version out to the width of another pointer. This is contrary to the spec and requires an explicit override. However, libsofthsm2 is using default packing everywhere except Windows, returning this (wrong!) platform-default padding. So if a client sets up their structs correctly, as soon as they try to call into a function from the list, they get a value that's part of the correct address and part an adjacent one, seg faulting on deref.

According to this issue, it seems to have become such a deeply ingrained assumption (originating in pk11-kit) it's now a de facto standard they refuse to fix. :roll_eyes:

So I guess there's a bit of a philosophical question to be answered here: follow the standard or the mob?

[ionut-arm]

Changing the packing for a struct is just a matter of adding an attribute to it at build time, right? Could we add a feature to control that, so that users are free to choose whichever suits them?

[vkkoskie]

That's right. However, I don't think there's a way to test the bindings since there's currently only one way to simulate the device. It would require matching compilation(s) of libsofthsm2. There's a preprocessor hook for that, but the current CI setup uses the image default instead of doing the compilation itself.

[jhagborgftx]

Copying this here from the original PR:

It seems that structure packing should be limited to Windows. This is a known inaccuracy of the spec, and there is a proposal to change this in v3.2. This was accepted without objection according to these meeting minutes.

Even in older versions of the spec, it seems nobody was actually packing structures on Unix. So following the spec literally will cause ABI incompatibility with every existing PKCS#11 implementation.

[jhagborgftx]

According to this issue, it seems to have become such a deeply ingrained assumption (originating in pk11-kit) it's now a de facto standard they refuse to fix. roll_eyes

AFAIK, it goes back much further than that. See this thread between TC members for some background.