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

Prefer Rust types to libc types #837

Closed
wants to merge 1 commit into from
Closed

Conversation

josephlr
Copy link
Contributor

@josephlr josephlr commented Jun 13, 2019

Part of addressing #744

For many types in libc (uint8_t, int32_t, etc...) we assume these types are equivalent to certain Rust types (u8, i32, etc...) even if libc is empty. This PR does three things:

  • Moves size_t to usize, as they are definitionally the same type. This is equivalence is currently assumed by ring anyway, and it is also assumed by Rust itself in things like memcpy.
  • Changes the C implementation of GFp_memcmp to return a u8 instead of a int. This fits better with the functions implementation, and the return value is only ever compared to zero anyway. As a side note, is there any reason this function cannot be written in Rust?
  • Cleans up the usage of libc to use the constants for the getrandom syscall. This has been in libc for a while, so we don't need to increment the libc version in Cargo.toml.

Now the only necessary C types are for bssl::Result and a bunch of one-off code in aead/aes.rs, which I'll address in a future CL.

@briansmith
Copy link
Owner

Thanks for the PR. I already considered doing this before and I rejected it because the assembly language code has tricky edge cases when sizeof(int) < sizeof(size_t) and I don't want to get out of sync with BoringSSL without a strong reason. Let's discuss potential solutions in issue 744.

@briansmith briansmith closed this Jun 13, 2019
@josephlr
Copy link
Contributor Author

I don't want to get out of sync with BoringSSL without a strong reason

That makes perfect sense, GFp_memcmp should be left as is. I also opened #839 that just contains the getrandom fixes. It doesn't change any types.

I already considered doing this before and I rejected it because the assembly language code has tricky edge cases when sizeof(int) < sizeof(size_t)

@briansmith this makes sense, and I agree that we fixing the corner cases around libc::c_uint and friends requires much more scrutiny. However, would you want to replace libc::size_t with the usize in just the rust code. These types are always the same, and this kind of thing is already common in ring. For example, we already use u8 instead of libc::uint8_t.

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

Successfully merging this pull request may close these issues.

2 participants