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

Bump bindgen to 0.69.4 #199

Merged
merged 2 commits into from Mar 13, 2024
Merged

Bump bindgen to 0.69.4 #199

merged 2 commits into from Mar 13, 2024

Conversation

hug-dev
Copy link
Member

@hug-dev hug-dev commented Feb 21, 2024

Bumps bindgen in order to also bump shlex which version we currently use has a security vulnerability.

Bumps bindgen in order to also bump shlex which version we currently use
has a security vulnerability.

Signed-off-by: Hugues de Valon <hugues.de-valon@einride.tech>
@ionut-arm
Copy link
Member

Ok, I've ran regenerate_bindings.sh locally, it seems only the Loongarch build fails for some reason. I've created rust-lang/rust-bindgen#2765 based on that.

Any chance you have the bandwidth to run that? Or if you give me push rights to your fork I can take care of it.

cc @heiher

@heiher
Copy link
Contributor

heiher commented Feb 21, 2024

Interesting. I can successfully cross generate these bindings locally, including LoongArch.

Steps:

# Install rust 1.76.0 (stable)
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh

git clone -b shlexed https://github.com/hug-dev/rust-cryptoki
# HEAD commit 9765405f7be9445aed972f24b41d5bb638bd12e8
cd rust-cryptoki
cargo build --features generate-bindings

cd cryptoki-sys
./regenerate_bindings.sh

Logs:

+ for target in $targets
+ rustup target list
+ grep -q 'loongarch64-unknown-linux-gnu (installed)'
+ rustup target install loongarch64-unknown-linux-gnu
info: downloading component 'rust-std' for 'loongarch64-unknown-linux-gnu'
info: installing component 'rust-std' for 'loongarch64-unknown-linux-gnu'
+ TARGET_INSTALLED=loongarch64-unknown-linux-gnu
+ cargo build --target loongarch64-unknown-linux-gnu --features generate-bindings
   Compiling cfg-if v1.0.0
   Compiling cryptoki-sys v0.1.7 (/home/hev/git/rust/rust-cryptoki/cryptoki-sys)
   Compiling libloading v0.7.4
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.46s
+ find ../target/loongarch64-unknown-linux-gnu/ -name pkcs11_bindings.rs
+ xargs -I '{}' cp '{}' src/bindings/loongarch64-unknown-linux-gnu.rs
+ '[' loongarch64-unknown-linux-gnu == loongarch64-unknown-linux-gnu ']'
+ rustup target remove loongarch64-unknown-linux-gnu
info: removing component 'rust-std' for 'loongarch64-unknown-linux-gnu'

Results:

diff --git a/cryptoki-sys/src/bindings/loongarch64-unknown-linux-gnu.rs b/cryptoki-sys/src/bindings/loongarch64-unknown-linux-gnu.rs
index 3854d02..81bdfa3 100644
--- a/cryptoki-sys/src/bindings/loongarch64-unknown-linux-gnu.rs
+++ b/cryptoki-sys/src/bindings/loongarch64-unknown-linux-gnu.rs
@@ -1,4 +1,4 @@
-/* automatically generated by rust-bindgen 0.66.1 */
+/* automatically generated by rust-bindgen 0.69.4 */
 
 pub const PKCS11_H: u32 = 1;
 pub const CRYPTOKI_VERSION_MAJOR: CK_BYTE = 2;

@ionut-arm
Copy link
Member

Interesting. That means that the problem is either with the Rust toolchain I was using (an older version), or with cross-compilaton. I'll check again.

@heiher
Copy link
Contributor

heiher commented Feb 21, 2024

Interesting. That means that the problem is either with the Rust toolchain I was using (an older version), or with cross-compilaton. I'll check again.

Thanks. The LoongArch support start from Rust 1.71.0.

@hug-dev
Copy link
Member Author

hug-dev commented Feb 21, 2024

Oh sorry, forgot about that! I got the same error running the script than Ionut, running with Rust 1.76.0.

@wiktor-k
Copy link
Collaborator

wiktor-k commented Feb 21, 2024

For the record it works on my machine (as in: ./regenerate_bindings.sh succeeds, loongarch is in the script output). (I had to adjust the target dir though as I'm using custom CARGO_TARGET_DIR).

@tgonzalezorlandoarm
Copy link
Member

Hi, thanks for the PR.

Parsec's, release (which is coming sometime around April) depends on rust-cryptoki's.
For easily packaging distros, we need alignment between the different dependencies' versions: Each crate should be brought, ideally, with only one version. Parsec also depends on bindgen as a dependency, and currently we are planning on releasing with version 0.66.1:

From what I've noticed in our research on the shlex vulnerability for Parsec, the concerning APIs were not being used on our bindgen calls, and shlex was only being used for testing on bindgen elsewhere.

I will have to check for this repository, but I think the same may possibly apply here.
If this is the case, I think there would be no urgency in upgrading bindgen to solve the shlex vulnerability as this one would not directly apply. I will confirm shortly if this is the case.

To align bingen versions between rust-cryptoki and parsec, it would be great if we could postpone this PR from being merged until parsec updates its bindgen version, which would happen after release.

Please let me know if this is okay or if there are any other questions/comments

@hug-dev
Copy link
Member Author

hug-dev commented Feb 21, 2024

Yes that is perfectly fine! I can close it for now and maybe replace it with an ignore flag on the cargo audit command so that the nightly is not failing anymore?
I can do that once it is verified that the vulnerable shlex API are not used!

@tgonzalezorlandoarm
Copy link
Member

I've proposed a way to update bindgen in Parsec as well before release and the team agreed, so this can be unblocked. Thanks a lot and sorry for the noise!

@hug-dev
Copy link
Member Author

hug-dev commented Feb 22, 2024

Can someone for who the script work out of the box take over this PR 🙏 It became more work than I thought to make it cross-compile

@ionut-arm
Copy link
Member

@hug-dev - yeah, no worries, I'll take it over

@ionut-arm
Copy link
Member

@heiher - I've tried again with the latest Rust compiler. I think the issue is with cross-compiling, I'm doing so from x86_64. Should I update this PR with all other bindings, and let you open a PR afterwards updating the loongarch bindings?

@heiher
Copy link
Contributor

heiher commented Feb 22, 2024

@heiher - I've tried again with the latest Rust compiler. I think the issue is with cross-compiling, I'm doing so from x86_64. Should I update this PR with all other bindings, and let you open a PR afterwards updating the loongarch bindings?

I have also cross-compiled on x86_64 before. I'll try to reproduce it using Github Action.

@heiher
Copy link
Contributor

heiher commented Feb 22, 2024

@heiher - I've tried again with the latest Rust compiler. I think the issue is with cross-compiling, I'm doing so from x86_64. Should I update this PR with all other bindings, and let you open a PR afterwards updating the loongarch bindings?

Thanks for everything you have done for LoongArch.

The root cause is that the libclang of the Ubuntu distribution is too old and does not support LoongArch, but my locally ArchLinux does. I propose to install the snapshot version from LLVM offical to solve it:

https://github.com/heiher/rust-cryptoki/blob/main/.github/workflows/ci.yml#L15-L19

      - name: Setup clang
        run: |
          wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add -
          sudo add-apt-repository -y 'deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy main'
          sudo apt install libclang-dev

Action results: https://github.com/heiher/rust-cryptoki/actions

Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
@ionut-arm
Copy link
Member

Excellent, thank you for pointing that out, @heiher ! I've now updated all the bindings.

@wiktor-k
Copy link
Collaborator

The root cause is that the libclang of the Ubuntu distribution is too old and does not support LoongArch, but my locally ArchLinux does.

Ah, that explains it, since I also use Arch, btw 😏 😉

@hug-dev
Copy link
Member Author

hug-dev commented Feb 29, 2024

Thank you so much @ionut-arm !! Can we now approve and merge?

@hug-dev hug-dev enabled auto-merge March 11, 2024 09:00
@hug-dev hug-dev merged commit 982d6bf into parallaxsecond:main Mar 13, 2024
7 checks passed
@hug-dev hug-dev deleted the shlexed branch March 13, 2024 11:00
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.

None yet

6 participants