Prevent cross-version linkage while allowing multiple versions of *ring* to be used in the same program

[briansmith]

I guess it is possible for people to somehow link two versions of ring into a program with unpredictable results. Part of the unpredictability of this is that the C/asm code in ring might have different ABI across versions, but the linker does not know that. Presumably it is less of an issue for Rust code because Rust function names are mangled and so if their ABI changes, the linker won't confuse two versions of a function.

It isn't ring's responsibility to prevent these issues from happening, but I have an idea that might help with this.

[alexcrichton]

FWIW This is a problem for all crates that have C dependencies, especially those that vendor the code! For example libgit2-sys builds and links in the libgit2 library that Cargo uses, and two major version of the libgit2-sys crate would cause lots of problems (lots of duplicate symbol with differing versions of libgit2).

For native dependencies at least Cargo has a links key in the manfiest which basically tells cargo "only one crate with this value of links can be in any crate graph". This is Cargo's way of giving that error message way before you ever reach the linker. With that key it's possible for ring to say, for example links = "ring_support" (or something like that) and that way Cargo will prevent two crates that link to ring_support being in the same crate graph, such as ring 0.8 and 0.9.

A downside of this approach, though, is that the error messages aren't the best. Historically, for example, Hyper's received a lot of bug reports when the openssl-sys crate has a major version bump. The actual problem was in totally different crates, but Hyper got the brunt of the issues as it's transitively the main point through which the openssl-sys crate was used.

Given the rate of releases for ring (which is great!) it'd be awesome for all versions to be able to coexist in the same binary so you don't have to upgrade the whole world all at once. The Rust compiler handles this (as you've found) by mangling the symbol names. Would it be possible for ring to do the same? Basically just append a "unique string" somewhere in each symbol name exposed by the library. If you're curious about that and need help on the build script and/or Rust side of things to avoid duplication and whatnot, I wouldn't mind helping out!

[briansmith]

Thanks for that explanation! I had no idea about that. I added the links to version 0.9.3.

As things currently stand, I only support the latest release of ring, so supporting programs containing multiple versions at once is out of scope. I already yanked all versions except {0.7.6, 0.8.1, and 0.9.4} and I plan on yanking 0.7.6 and 0.8.1 RSN.

[briansmith]

OK, I think everything we need to do is done here.

[alexcrichton]

Out of curiosity, do you have an idea for how much work it would take to lift the restriction of requiring links?

Right now they way that version resolution works out means that if you have a major version upgrade of a dependency with a links key it means that you yourself also need a major version upgrade. For example the cookie crate depends on ring internally but whenever ring makes a major version bump it means that cookie itself must also make a major version bump. With the pace of breaking changes in ring this makes it difficult to keep up!

If it's easy-ish to lift the restriction by requiring links that'd be ideal because then multiple versions of ring could be linked into the same program, meaning that cookie could make major version bumps of ring without itself requiring a major version bump.

[briansmith]

I only support the latest version of ring, so linking two versions of ring into the same program is an unsupported configuration which means we don't expend effort on it. We did a couple of experiments showing that the major version bumps don't seem to cause significant problems for downstream users. So, it seems like something that at first would be nice to do, but AFAICT it's a net negative.

[ctz]

It causes a problem if some end user wants to build an executable using cookie and (say) rustls -- both crates have to synchronise their incompatible-version release schedule with each other in order to coexist in the same end-executable crate. See also rustls/rustls#86

[SergioBenitez]

@briansmith Being able to link two versions of ring into one binary is incredibly important, and it will become even more important as ring gains adoption. In fact, being able to link two versions at once was the primary reason why I used ring as opposed to openssl in my projects. I want to illustrate why in hopes that you reconsider.

The current state of affairs forces a lock-step upgrade of dependencies. Consider what Rocket had to do to upgrade from cookie 0.8 to cookie 0.9, which updated ring to 0.11. First, the cookie dependency was updated to 0.9. This caused a breakage since Rocket also depends on rustls and hyper-rustls which depended on ring 0.9. I maintain a fork of hyper-rustls, but I couldn't update it to ring 0.11 because it depends on rustls. So, I opened up an issue on rustls. rustls itself depended on webpki-roots, which depends on ring. So, webpki-roots had to be updated. Thankfully, it had no downstream uses of ring, so it updated just fine. Once webpki-roots was updated, rustls was updated. Once rustls was updated, hyper-rustls was updated. Finally, once hyper-rustls was updated, I could update the three ring-related dependencies in Rocket. To upgrade one dependency in Rocket, I had to force updates to 3 other crates. All because of ring.

Thankfully, @ctz owns rustls and webpki-roots, and I maintain cookie, so this was able to happen rather quickly. What if this hadn't been the case? How long would it have taken for the upgrade to happen? Your goal is for users to always be on the latest version of ring, but I think you'll see just the opposite as users are forced to the least common denominator.

Additionally, ring is most likely to be used as an internal library. It's unlikely that two libraries will interface through ring's types. This is where Cargo/Rust's ability to have two versions of the same library in one project shines! It's a shame that we've lost this benefit. And, what's worse, ring now directly controls when a library has to release a breaking change. This is a massive inversion of control; I really don't want a dependency controlling my version numbers. Again, I think you'll see many projects refusing to update ring as a result, leading to libraries on old versions of ring.

[briansmith]

I think the current state is fine and I frankly don't want to spend much effort on this. However, that said, perhaps other people are willing to spend effort to come up with a reasonable solution. Here are the requirements for the feature:

Must be Automatic, except possibly for small configuration changes just prior to cargo publish

The solution should be automatic. Anybody that clones ring from GitHub should be able to cargo build with no additional configuration and have it work. During releasing buidls to crates.io, I'm willing to tolerate manually changing an extra setting or two during actual releases, much in the same way we have to manually bump the version number. However, we shouldn't need to make mass edits to the source code to, for example, rename symbols. Further, we must not need to keep track of whether we made an "ABI-breaking" change in the C/asm code.

One crate

ring should remain one crate. No splitting the C/asm code into a separate "-sys" crate. My understanding is that splitting it into multiple crates wouldn't really solve any problems anyway. We will be frequently changing the C and asm code.

Easy source code cross-referencing

The names under which C/asm symbols are referenced in the Rust code must not change over releases. For example, every use of GFp_AES_encrypt in the Rust code must use the name GFp_AES_encrypt even if the library actually exports it with some versioned name like GFp_AES_encrypt_v12345. It would be OK if we had to replace the extern { fn GFp_AES_encrypt(...) } declaration with the use of a macro something like this:

Current:

extern {
        fn GFp_AES_encrypt(in_: *const u8, out: *mut u8, key: *const AES_KEY);
}

Reasonable alternative:

versioned_extern!(GFp_AES_encrypt(in_: *const u8, out: *mut u8, key: *const AES_KEY));

that expands to something like this:

extern {
    #[link_name = "GFp_AES_encrypt_v12345"]
    fn GFp_AES_encrypt(in_: *const u8, out: *mut u8, key: *const AES_KEY);
}

In particular, when ring is checked out from git (but not necessarily in a released version from crates.io), CLion with the Rust plugin installed must be able to find all the users of GFp_AES_encrypt in the Rust code and must be able to navigate to the extern declaration just like currently. RLS-based tools must eventually be able to do likewise, though it would be OK for me if this were deferred to later if RLS itself requires modifications for the solution to work.

Similarly, the names of the symbols in the C/asm source code must not change. In particular the definition of the function must use the same name as is used in the Rust code, for easy cross referencing.

void GFp_AES_encrypt(const uint8_t *in, uint8_t *out, const AES_KEY *key) {
    ....
}

It would be OK if this needed to be rewritten to something like :

void GFp_extern(GFp_AES_encrypt)(const uint8_t *in, uint8_t *out, const AES_KEY *key) {

In particular, when ring is checked out from Git (but not necessarily using a version from crates.io) then CLion and Visual Studio 2017 should be able to find all the uses of a function from C code using its plain name "GFp_AES_encrypt" like they currently can, and they both should be able to navigate to the definition (when the function is defined in C) and the declaration of the function like they currently can. For functions defined in assembly language, it must be easy to navigate to all the definition using the unversioned named with normal Shift-Ctrl+F-style searching.

It would probably be OK if we had to have a single file where we have a bunch of #defines like this:

#define GFp_AES_encrypt GFp_VERSIONED_NAME(GFp_AES_encrypt)
#define GFp_some_other_function GFp_VERSIONED_NAME(GFp_some_other_function)
...

Must work well on All Currently-supported Platforms

In particular, note that some important contributors to ring use Windows as a primary environment and so solutions that don't work on Windows aren't acceptable. Conversely, most users of ring are using macOS and/or Linux so solutions that only work on Windows aren't acceptable. However, the implementation of the solution can vary per platform if necessary, though ideally it would not.

Debugging must work

Unlike the in-IDE-editing experience, it is fine if the debugger deals with versioned names. But debugging must work.

Must not require any new build-time tools.

In particular, don't add dependencies on Perl or Go or Make or whatever. (Note, in particular, that Perl is not required to build ring releases from crates.io, though it is required for git checkouts currently; I hope to get rid of the Perl requirement for Git checkouts sometime.)

Somebody other than Brian Smith Must Write the PR

I will review the changes but for various reasons it's not realistic for me to do the work myself.

[SergioBenitez]

Ok. I'm on it.

[TerrariumCode]

Running into the same issue found here - rwf2/Rocket#492

Is there any kind of ETA on this? Would love to get a project I have to move forward with this. Appreciate the effort 👍

[AdrianChallinorOsiris]

I have been chasing this conversation, going in circles, for an hour now!

This is important. I want to upgrade the version of Rocket, but other libraries have dependencies on Ring that cause a conflict.

Have we inadvertently reinvented a version of DLL-HELL in Rust?

[ramosbugs]

@SergioBenitez @1wilkens: Has there been any progress on this ticket? If someone can provide an update on the state of the items @briansmith suggested here, I'm willing to help out to drive this to completion. I think it's pretty critical for the Rust ecosystem.

[1wilkens]

My Travis CI changes and hacky python script are still in a PR somewhere. Until now I didn't check BoringSSL's prefixing scheme, as I stopped using rocket myself (in part because of the versioning problems) but I did that now.
It seems like it's mostly some bash scripts as documented here. But since ring doesn't build BoringSSL via the "native" installer which features the prefix option, I am not sure how to integrate this with ring.
I'll try to dig out my PR against @SergioBenitez' branch and open that as a single WIP PR to ring itself and maybe we can discuss some more then.

[ramosbugs]

thanks for the update! I have a branch (https://github.com/ramosbugs/ring/tree/version_symbols) that's almost working with BoringSSL's changes, but with @briansmith's preferred approach of pregenerating any of the files that require Go/Perl tools. I should have a PR out soon, hopefully later today.

[LeviSchuck]

If you've come here because you hit this problem with rocket, see below.

If you are not using the secure cookies feature in rocket, you can disable it and therefore the tied 0.13.5 ring dependency with the following in your Cargo.toml

[dependencies.rocket]
version = "0.4.2"
default-features = false

[briansmith]

I'm not even sure which issue is supposed to be tracking this problem any more, but PR #1267 aims to ensure that ring 0.17 .x can coexist with an earlier version of ring in the same executable, and it lays the groundwork for a fully automated solution.

[briansmith]

I'm going to close this issue. A long time ago I did implement a solution that does "prevent cross-version linkage." Now most people are interested in this issue because they didn't like that solution. Issue #1268 outlines the new solution and links to the PR that will allow the problems to be solved.