Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use BitLength in FFI when transparent structs are supported #345

Closed
briansmith opened this issue Nov 14, 2016 · 6 comments
Closed

Use BitLength in FFI when transparent structs are supported #345

briansmith opened this issue Nov 14, 2016 · 6 comments

Comments

@briansmith
Copy link
Owner

Example before:

        fn GFp_AES_set_encrypt_key(key: *const u8, bits: usize,
                                   aes_key: *mut AES_KEY) -> c::int;

Example after:

        fn GFp_AES_set_encrypt_key(key: *const u8, bits: BitLength,
                                   aes_key: *mut AES_KEY) -> c::int;

This depends on rust-lang/rfcs#1758.

@briansmith
Copy link
Owner Author

The rust feature is being tracked in rust-lang/rust#43036

@pietro
Copy link
Contributor

pietro commented Jan 15, 2019

The FFI functions use c::uint instead of usize. I don't think there's any length that would overflow u32 but on most 64-bit platforms we would still convert from u64 to u32 (I think).

@briansmith
Copy link
Owner Author

The FFI functions use c::uint instead of usize. I don't think there's any length that would overflow u32 but on most 64-bit platforms we would still convert from u64 to u32 (I think).

Yes. Unfortunately it is not trivial to switch them to usize. I think we can/should just leave those function calls the way they are because there is no real risk of screwing them up the way aes.rs is now written.

@briansmith
Copy link
Owner Author

In particular, I don't want to change the representation of BitLength to be c::uint. My long-term plan is to remove all uses of non-usize/size_t variable-sized integers (c::int and c::uint in particular) from the codebase.

@pietro
Copy link
Contributor

pietro commented Jan 16, 2019

I added the #[repr(transparent)] to BitLenght and changed the functions on aes.rs to use BitLenght instead of c::uint in pietro/ring@0db50fa.

@briansmith
Copy link
Owner Author

In BoringSSL, David Benjamin did some analysis and determined that, in general, we have to be very careful in changing the int, unsigned, size_t, etc. types in assembly language function declarations. In particular, if a function is expecting an unsigned but we change the declaration to usize then it might incorrectly zero out the high bits of the argument (if usize is larger than int/unsigned). In the case of set_encrypt_key it probably doesn't matter because the valid values are well within the range of unsigned, but also I don't see a lot of value in changing the declaration.

More generally, we now very rarely pass bit lengths to the C/asm code so I don't think it's worth even thinking about this as a TODO item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants