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

Allow multiple versions of *ring* to coexist in the same executable #1268

Closed
briansmith opened this issue May 3, 2021 · 10 comments
Closed

Comments

@briansmith
Copy link
Owner

Currently, one cannot build an app that uses more than one version of ring because ring uses the Cargo.toml "links" mechanism to prevent it. Although only the latest version of ring is supported at any one time, and everybody is strongly recommended to use only the latest version, this limitation makes it really difficult to incrementally upgrade dependencies to a new version of ring.

My plan is to use the same value of "links" in Cargo.toml for every semver-compatible version. So, for example, ring 0.17.x will likely use links="ring_core_0_17", ring 0.18.x will probably use links="ring_core_0_17", etc. This would still end up preventing using ring 0.17.1 and 0.17.2 (for example) in the same program but it will allow using any one 0.17.x version along with any one 0.18.0 version, for example. In practice, this seems to be exactly the level of flexibility that people need.

PR #1267 starts the implementation of the solution. It doesn't implement automation for choosing the prefix but it does automate (nearly) everything once the prefix is edited. It doesn't implement the check that we haven't missed any symbols as part of the renaming.

Very soon, I will implement additional automation to verify that no prefixing was skipped, that will run for each PR, and that will run as part of the release process.

In the (hopefully near) future, I hope to add automation that derives the prefix to use from the ring version number, but this isn't urgent. Already most of the automation other than this is done in PR #1267.

Modulo a few details, the implementation strategy is about the same as what BoringSSL natively supports and seems to be similar to the previous, aborted, attempt to solve this problem.

@briansmith
Copy link
Owner Author

briansmith commented May 3, 2021

The solution in #1267 requires that the prefix is updated in multiple places:

It would be great to use env!("CARGO_MANIFEST_LINKS") + "_" within build.rs to eliminate needing to manually update build.rs when the prefix is updated. I haven't done that yet because that would not work for the "pregenerated_asm" mechanism. Alternatively, I suppose build.rs could just read it out of Cargo.toml itself; then it could also ensure that crate version number is consistent with the prefix. Suggestions regarding this are appreciated.

It would also be nice to find a mechanism for passing the value of the prefix from build.rs to src/prefixed.rs automatically so that src/prefixed.rs doesn't need to be updated manually. I bet somebody knows a solution for doing this; a tip about how to go about it would be appreciated.

Note that I've intentionally punted on solving this problem for non-release versions (i.e. direct Git checkouts) of ring for the time being.

@briansmith
Copy link
Owner Author

I updated PR #1267 to add a basic check that all necessary symbols are prefixed. This found a bug where we were emitting code on ARM targets for an AES decrypt function that we didn't need, so it is good to have the check now. However, the check is a script that is based on nm and Apple's nm seems to trip over the output of Rustc for aarch64 targets, so I've had to disable the check in that situation. And because Windows doesn't ship with an nm, I had to disable the check for Windows. BoringSSL has a Golang utility that does something similar, so we might be able to switch to that instead, in the future.

@briansmith
Copy link
Owner Author

However, the check is a script that is based on nm and Apple's nm seems to trip over the output of Rustc for aarch64 targets, so I've had to disable the check in that situation. And because Windows doesn't ship with an nm, I had to disable the check for Windows. BoringSSL has a Golang utility that does something similar, so we might be able to switch to that instead, in the future.

Alternatively, we can just install llvm-tools-12 and use that to do this check, on every platform.

@jplatte
Copy link

jplatte commented May 3, 2021

It would also be nice to find a mechanism for passing the value of the prefix from build.rs to src/prefixed.rs automatically so that src/prefixed.rs doesn't need to be updated manually. I bet somebody knows a solution for doing this; a tip about how to go about it would be appreciated.

I can think of two possibilities:

  • Instruct cargo to set an environment variable for the build (docs):
    println!("cargo:rustc-cfg=RING_FFI_SYMBOL_PREFIX={}", prefix_value);
    and change the concat! invocation to concat!(env!("RING_FFI_SYMBOL_PREFIX"), stringify!($name))
  • Write the prefix to a file in $OUT_DIR:
    let prefix_path = Path::new(std::env("OUT_DIR").unwrap()).join("ring_ffi_symbol_prefix.txt");
    fs::write(prefix_path, prefix_value)?;
    and change the concat! invocation to concat!(include_str!(concat!(env!("OUT_DIR"), "ring_ffi_symbol_prefix.txt")), stringify!($name))

@jplatte
Copy link

jplatte commented May 3, 2021

Alternatively, I suppose build.rs could just read it out of Cargo.toml itself; then it could also ensure that crate version number is consistent with the prefix. Suggestions regarding this are appreciated.

Sounds good. What kinds of suggestions are you looking for?

@briansmith
Copy link
Owner Author

println!("cargo:rustc-cfg=RING_FFI_SYMBOL_PREFIX={}", prefix_value);

@jplatte Thanks so much! I decided to go with this approach because it seemed to perform better. Maybe it was just noise but the approach of reading the prefix from a file seemed to make the build notably slower. Otherwise, I would have preferred that approach since, as part of the verification stage of CI that checks that we didn't forget to prefix any symbols, I really could use the prefix written to a file.

@briansmith
Copy link
Owner Author

The PR to eliminate the hard-coded value of the prefix in src/prefixed.rs is PR #1270.

@briansmith
Copy link
Owner Author

briansmith commented May 3, 2021

It would be great to use env!("CARGO_MANIFEST_LINKS") + "_" within build.rs to eliminate needing to manually update build.rs when the prefix is updated. I haven't done that yet because that would not work for the "pregenerated_asm" mechanism.

I figured out a solution. PR #1271 changes the way the precomputed assembly is done so now this is no problem. PR #1272 derives the prefix from the "links" field in Cargo.toml so now we'll only need to modify Cargo.toml (version and links fields) for each release. PR #1273 fixes a typo from PR #1271.

My plan is to use the same value of "links" in Cargo.toml for every semver-compatible version. So, for example, ring 0.17.x will likely use links="ring_core_0_17", ring 0.18.x will probably use links="ring_core_0_17", etc. This would still end up preventing using ring 0.17.1 and 0.17.2 (for example) in the same program but it will allow using any one 0.17.x version along with any one 0.18.0 version, for example. In practice, this seems to be exactly the level of flexibility that people need.

Now I'm planning to have a different links value for each version, which will be more flexible.

@briansmith
Copy link
Owner Author

I updated PR #1267 to add a basic check that all necessary symbols are prefixed. This found a bug where we were emitting code on ARM targets for an AES decrypt function that we didn't need, so it is good to have the check now. However, the check is a script that is based on nm and Apple's nm seems to trip over the output of Rustc for aarch64 targets, so I've had to disable the check in that situation. And because Windows doesn't ship with an nm, I had to disable the check for Windows. BoringSSL has a Golang utility that does something similar, so we might be able to switch to that instead, in the future.

The follow-up work to enable the check for targets for which it is currently disabled is issue #1275.

Other than that follow-up, after the PRs linked above are merged, this will be complete.

@briansmith
Copy link
Owner Author

Most users of Bazel rewrite build.rs in some better, Bazel-y way. The recent changes to build.rs, especially the ones here, will probably eventually require changes to those Bazel config files. I'd love to see somebody contribute a Bazel configuration that we can maintain within this repo and that others could reuse; if we had that then I could have modified it as part of these PRs. See issue #1243 if you're interested in contributing Bazel support.

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

2 participants