mheese / rust-pkcs11

Rust PKCS#11 Library
Apache License 2.0
75 stars 33 forks source link

types: Fix CK_LONG/CK_ULONG on Linux ARM32 #40

Closed woodruffw closed 4 years ago

woodruffw commented 4 years ago

This fixes an ABI mismatch that I observed experimentally when trying to use rust-pkcs11 with a Nitrokey on a 32-bit ARM host (really 64-bit, but it's a Raspberry Pi running 32-bit Raspbian).

There are probably other ABI mismatches on other target OSes and architectures; a more general fix here might be to use pkcs11t.h directly via bindgen or some other mechanism.

codecov-commenter commented 4 years ago

Codecov Report

Merging #40 into master will decrease coverage by 0.02%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
- Coverage   65.43%   65.41%   -0.03%     
==========================================
  Files           4        4              
  Lines        3446     3444       -2     
==========================================
- Hits         2255     2253       -2     
  Misses       1191     1191              
Impacted Files Coverage Δ
src/types.rs 37.06% <ø> (ø)
src/lib.rs 65.12% <0.00%> (-0.04%) :arrow_down:
src/tests.rs 80.41% <0.00%> (-0.02%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d66fee4...cc637db. Read the comment docs.

hug-dev commented 4 years ago

Wow, I have always thought that the PKCS 11 types would be typedefed over stdint.h types 😅 That is indeed an interesting problem.

I would be in favor of producing (and commiting in-tree) various bindings for a set of target triplets. Also it is worth noting @joechrisellis PR's on bindgen to be able to use libloading directly with the bindings produced.

Brainstorming here, but this PR + the need of safety given by abstraction (#38) make me wonder if it would be worth following the Rust idiom of spliting pkcs11 in a low-level pkcs11-sys crate for FFI bindings and dynamic loading and a high-level pkcs11 one for idiomatic and safe Rust types/methods.

woodruffw commented 4 years ago

Wow, I have always thought that the PKCS 11 types would be typedefed over stdint.h types 😅 That is indeed an interesting problem.

Unfortunately not 🙃 -- they're defined over the normal C primitive types, with additional (not enforced?) assumptions about being at least 32 bits for e.g. CK_ULONG.

mheese commented 4 years ago

good catch... and yes, bindgen is probably the right way out of this. pkcs11 is unfortunately full of "C"isms that make the life in rust really hard

mheese commented 4 years ago

@hug-dev as you are obviously interested in ARM, would you mind researching (quickly) if we can add a build target for that?

hug-dev commented 4 years ago

@hug-dev as you are obviously interested in ARM, would you mind researching (quickly) if we can add a build target for that?

I think we should either have bindgen generating at build-time the bindings for the specific target or (better) pre-generate the bindings and choose wich one to use depending on the target. I would say for now it is better to merge this fix and use bindgen after rust-lang/rust-bindgen#1846 is merged. Regarding the target I believe it would be arm-unknown-linux-gnueabi?

woodruffw commented 4 years ago

Regarding the target I believe it would be arm-unknown-linux-gnueabi?

Yes, I believe that's correct. If you need the specific variant, it's armv7l, but that shouldn't be part of the triple.