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

FFI C code should use uint32_t not size_t for u32 Rust parameters. #1791

Closed
thomcc opened this issue Aug 25, 2021 · 3 comments
Closed

FFI C code should use uint32_t not size_t for u32 Rust parameters. #1791

thomcc opened this issue Aug 25, 2021 · 3 comments

Comments

@thomcc
Copy link
Contributor

thomcc commented Aug 25, 2021

While going over the crates in my onboarding session, I noticed that our FFI layer uses size_t from C and u32 from Rust. One example of several:

https://github.com/ockam-network/ockam/blob/ffadf0633adcae4d0a061fa322fb17dd15afe6e6/implementations/rust/ockam/ockam_ffi/src/vault.rs#L60

https://github.com/ockam-network/ockam/blob/ffadf0633adcae4d0a061fa322fb17dd15afe6e6/implementations/rust/ockam/ockam_ffi/include/vault.h#L72

This is both not guaranteed to work, and can cause major issues in general on 64 bit platforms with ABIs that will pack consecutive integers into the same register when they fit (e.g. x86_64, for example) — we're largely saved by the fact that we don't have any functions that use two consecutive size_t arguments (instead, they're typically surrounded by pointers, which prevents packing the two INTEGER-class parameters into the same register on x86_64, and likely the same on other ABIs).

The right type to use here is usize from the Rust, which should be a simple change.

EDIT: Actually given that in theory there are platforms where this isn't guaranteed to be the case (see rust-lang/rust#88340 and https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/.60usize.60.20vs.20.60size_t.60), I think this should be changed so that C uses uint32_t in the code, rather than adjusting the Rust. Sorry for the confusion.

@thomcc thomcc changed the title FFI code should use usize not u32 for size_t parameters. FFI C code should use uint32_t not size_t for u32 Rust parameters. Aug 25, 2021
@Hraesvel
Copy link
Contributor

Hraesvel commented Aug 27, 2021

This seems a simple enough task. I've noticed a few other functions where a length is being passed as a size_t. I assume these should receive the same treatment where size_t is used for u32?

This is also my first contribution should this be a chore(rust) commit?

@mrinalwadhwa
Copy link
Member

Hi @Hraesvel thank you for picking this 🙏

should this be a chore(rust) commit

It should be a fix(rust): commit.

I've noticed a few other functions where a length is being passed as a size_t. I assume these should receive the same treatment where size_t is used for u32

Yes.

Looking forward to merging you first contribution 🎉

@mrinalwadhwa
Copy link
Member

Also when you send the PR ... could you accept our CLA

Read the CLA at
https://www.ockam.io/learn/how-to-guides/contributing/cla/

To accept the CLA please send a different PR to our contributors repo indicating that you accept the CLA by adding your Git/Github details in a row at the end of the CONTRIBUTORS.csv file:
https://github.com/ockam-network/contributors/blob/master/CONTRIBUTORS.csv

Thank you!!

Hraesvel added a commit to Hraesvel/ockam that referenced this issue Aug 27, 2021
Hraesvel added a commit to Hraesvel/ockam that referenced this issue Aug 27, 2021
fix  build-trust#1791

FFI C code should use uint32_t not size_t for u32 Rust parameters
@jdspdx jdspdx closed this as completed in 55d0acb Sep 13, 2021
jdspdx pushed a commit that referenced this issue Sep 27, 2021
fix  #1791

FFI C code should use uint32_t not size_t for u32 Rust parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants