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

gcm_context size mismatch for target i686-linux-android #299

Open
osobiehl opened this issue Jul 16, 2023 · 6 comments
Open

gcm_context size mismatch for target i686-linux-android #299

osobiehl opened this issue Jul 16, 2023 · 6 comments

Comments

@osobiehl
Copy link

I am trying to compile for the i686-linux-android target. However, it looks like there is a mismatch between the size of structs between the generated gcm_context struct and __SIZE_OF_GCM_CONTEXT.

Here is a quick overview of the build script I am using (NDK_HOME) is the path to android-ndk on system

export CFLAGS="--sysroot ${NDK_HOME}/toolchains/llvm/prebuilt/linux-x86_64/sysroot"
export TARGET_AR="${NDK_HOME}/toolchains/llvm/prebuilt/linux-x86_64/bin/llvm-ar"
export CC_i686_linux_android="${NDK_HOME}/toolchains/llvm/prebuilt/linux-x86_64/bin/i686-linux-android27-clang"
cargo build --target i686-linux-android --release

image

@Taowyoo
Copy link
Collaborator

Taowyoo commented Jul 18, 2023

Hi @DrTobe , I guess this error is related to : 612e36c . Did you remember why you made this change?

It seems that that commit hard coded the size of many C types (which may change in different platform) when computing the size of contexts.

And more importantly, the size of these contexts actually are not able to be hard coded since the type definitions in C side can change with different config.

@DrTobe
Copy link
Contributor

DrTobe commented Jul 19, 2023

The reasoning for these semi-hardcoded sizes is explained in the comment above:

// If the C API changes, the serde implementation needs to be reviewed for correctness. The
// following (unused) functions will most probably fail to compile when this happens so a
// compilation failure here reminds us of reviewing the serde impl.

Actually, I have tried to make this a little bit more platform-independent. Before, the sizes were fully-hardcoded with a 64-bit platform in mind. I changed these sizes to be dynamic in commit 5027a7c but that defeats the purpose of these checks. So I tried to calculate these sizes in a platform-independent way. Apparently, this does not work for some platforms.

Actually, it is unnecessarily painful to calculate C type sizes by hand for different platforms. Should we try to find a completely different approach here which avoids manual size calculations? Maybe, we could copy+translate the expected C types to Rust with a #[repr(C)] attribute. Then, we could just compare the sizes of the copied Rust types and the actual C types. Whenever the C types change, these should be different.

@DrTobe
Copy link
Contributor

DrTobe commented Jul 19, 2023

@osobiehl These functions are completely unused and just for compile-time checks. If you just want the code to compile, remove these functions and try again.

@Taowyoo
Copy link
Collaborator

Taowyoo commented Jul 19, 2023

@DrTobe I see what you mean here.
And regarding the different approach you mentioned, I think it can be achieved by having a copy if the bindgen rust code here, so we could use it for checking if C API is changed

@DrTobe
Copy link
Contributor

DrTobe commented Jul 19, 2023

That sounds smart but there is still the problem you mentioned above:

And more importantly, the size of these contexts actually are not able to be hard coded since the type definitions in C side can change with different config.

How can we handle the config changes then?

@Taowyoo
Copy link
Collaborator

Taowyoo commented Jul 19, 2023

I think my words about config changes was wrong.
Because with a specific version of mbedtls-sys , there is no feature about changing the config that will results the change on definition of those contexts.
So that's not a problem, and if a different version of mbedtls-sys is using with different definition of contexts, then it fails in the case why we need these checks here.

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

No branches or pull requests

3 participants