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

Version FFI symbols to allow cross-version linking #849

Closed
wants to merge 9 commits into from

Conversation

ramosbugs
Copy link

This PR addresses #535 by adopting the approach described in #619 (comment):

  • Merges remaining upstream BoringSSL prefix changes
  • Implements a versioned_extern! macro without adding another crate dependency
  • Uses pregenerated headers to avoid adding build-time Perl/Go dependencies

The mk/package.sh script pregenerates the required headers using a two-phase build similar to how https://github.com/google/mundane builds its prefixed symbols (but without changing how build.rs works). The AppVeyor and Travis CI tests run without versioned symbols, using a new boringssl_no_prefix Cargo feature.

Depends on #848.

@ramosbugs
Copy link
Author

Since the pre-generated asm files weren't being checked into git before, this PR continues that approach and doesn't check the pre-generated versioned symbol headers or symbols.txt file into git either. Is this the preferred approach, or should these files be checked in with @1wilkens's test to detect staleness?

@ramosbugs
Copy link
Author

@briansmith: any chance we can get this merged soon to avoid further merge conflicts?

@briansmith
Copy link
Owner

@briansmith: any chance we can get this merged soon to avoid further merge conflicts?

I hope to have this and the other build system improvements done in about 2 weeks.

jbg referenced this pull request in rustls/rustls Jul 24, 2019
@jbg
Copy link

jbg commented Aug 7, 2019

@briansmith would it be helpful for getting this merged if I went through and resolved the conflicts in this PR?

I’m currently in “DLL hell” yet again thanks to this bug – two of my dependencies have added features I need to make use of, but they’ve also upgraded to ring 0.16, while four other dependencies are still on ring 0.14.

Until this bug is resolved, I have to fork the aforementioned two dependencies to revert the ring upgrade, and maintain my forks until the other four dependencies upgrade, which can take months.

If I had unlimited time, I’d instead fork the other four dependencies and upgrade them myself, but generally that takes significant resources which I can’t spare right now. I’ve contributed code to upgrade one of them (rustls) but of course contributing code doesn’t immediately lead to a release, so I would still end up maintaining forks, potentially for a significant period of time.

ring is the only crate out of our hundreds of dependencies, many with multiple versions linked, that has ever caused us this type of problem. It will continue to cause this problem for everyone who uses multiple crates that depend on ring, every time you make a new release, until this PR is merged or some other solution to this problem is implemented, and the problem will only get worse as more crates make use of ring.

If fixing the conflicts in this PR doesn’t help, I’m happy to do anything I can to help this problem get solved expeditiously, if you can let me know how you would ideally like it solved.

@briansmith
Copy link
Owner

It's better to have the original author do all the work here to keep the IPR situation.

ring 0.16 has important fixes and improved side channel attack defenses that aren't in 0.14. So, when this bug is fixed, everybody should upgrade to 0.16. This feature won't save you any work between 0.14 and 0.16. Either way, it would be great if we could work together to upgrade your four dependencies to 0.16.

That said, I know that this is an important bug to fix, so once I'm done helping projects upgrade to 0.16, I will come back to this issue. But, this feature isn't a substitute for helping people upgrade to 0.16. Thus, the fastest way for people to get this issue resolved would be to help upgrade other libraries to use 0.16.

@briansmith
Copy link
Owner

To expand on this, I spent quite a lot of time helping Rustls upgrade to 0.16. If somebody else would have done that, then I could have had time to work on this. The same pattern will probably occur with Trust-DNS and jsonwebtoken and other dependencies. If somebody can take over upgrading those, then I probably would have time this weekend to deal with this issue.

@jbg
Copy link

jbg commented Aug 7, 2019

OK, thanks very much for the clarity, and indeed for all your efforts here. I’ll direct my efforts towards upgrading deps to 0.16.

@ramosbugs
Copy link
Author

It's better to have the original author do all the work here to keep the IPR situation.

Should I go through and resolve these conflicts again then? Do we expect any more to come along before this gets merged?

src/digest/sha1.rs Outdated Show resolved Hide resolved
src/digest/sha2.rs Outdated Show resolved Hide resolved
src/rand.rs Outdated Show resolved Hide resolved
@@ -54,17 +54,19 @@ set PATH=%USERPROFILE%\.cargo\bin;%cd%\windows_build_tools;%PATH%

if [%Configuration%] == [Release] set CARGO_MODE=--release

REM Disable BoringSSL symbol prefixing for CI runs w/o pregenerated files.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is our CI supposed to test the prefixing then?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you like me to add the Go toolchain to CI? That'll probably add considerable build time, but I can do that if you'd like

Cargo.toml Outdated Show resolved Hide resolved
) -> Command {
let mut c = cc::Build::new();
let _ = c.include("include");
if std::env::var("CARGO_FEATURE_BORINGSSL_NO_PREFIX").is_err() {
let _ = c.include(format!("{}/{}", PREGENERATED, SYMBOL_PREFIX_INCLUDE));
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather have this logic within base.h, which is included by every C file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just adds the includes directory to the search path. it doesn't actually include any of the headers. I think without this conditional, the build fails when the directory doesn't exist. I can guard it with a check for the directory existing instead of the environment variable, if you prefer.

src/rand.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
module boringssl.googlesource.com/boringssl
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, there are dependencies on the upstream Go source for reading the symbol names. if I remove this, we get:

go run util/read_symbols.go -out symbols.txt target/debug/build/ring-25e4c3b394b2836b/out/libring-core.a
build github.com/ramosbugs/ring/util: cannot find module for path boringssl.googlesource.com/boringssl/util/ar

.into(),
"symbols.txt".into(),
];
run_command_with_args(&get_command("GO_EXECUTABLE", "go"), &args);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that go is required now to build ring? Or has this already been the case? I'd be cautious with introducing dependencies on entire new programming languages.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O never mind it is only being run during packaging: 25add85

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that go is required now to build ring? Or has this already been the case? I'd be cautious with introducing dependencies on entire new programming languages.

I had the same question. We need to make certain that Go isn't used as part of the build, only part of packaging/pregeneration.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, it's not part of the build that occurs when people import the packaged crate

src/rand.rs Outdated Show resolved Hide resolved
src/cpu.rs Outdated Show resolved Hide resolved
src/cpu.rs Outdated Show resolved Hide resolved
Copy link
Author

@ramosbugs ramosbugs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll work on addressing the comments this weekend

) -> Command {
let mut c = cc::Build::new();
let _ = c.include("include");
if std::env::var("CARGO_FEATURE_BORINGSSL_NO_PREFIX").is_err() {
let _ = c.include(format!("{}/{}", PREGENERATED, SYMBOL_PREFIX_INCLUDE));
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just adds the includes directory to the search path. it doesn't actually include any of the headers. I think without this conditional, the build fails when the directory doesn't exist. I can guard it with a check for the directory existing instead of the environment variable, if you prefer.

@@ -56,6 +56,10 @@

// This file should be the first included by all BoringSSL headers.

#if defined(BORINGSSL_PREFIX)
#include <boringssl_prefix_symbols.h>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@briansmith: I am including the symbol headers in base.h. I'm confused about what else you'd like me to include in this file...

.into(),
"symbols.txt".into(),
];
run_command_with_args(&get_command("GO_EXECUTABLE", "go"), &args);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, it's not part of the build that occurs when people import the packaged crate

@@ -54,17 +54,19 @@ set PATH=%USERPROFILE%\.cargo\bin;%cd%\windows_build_tools;%PATH%

if [%Configuration%] == [Release] set CARGO_MODE=--release

REM Disable BoringSSL symbol prefixing for CI runs w/o pregenerated files.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you like me to add the Go toolchain to CI? That'll probably add considerable build time, but I can do that if you'd like

src/digest/sha1.rs Outdated Show resolved Hide resolved
@jbg
Copy link

jbg commented Aug 14, 2019

@ramosbugs Rust’s name mangling also prevents conflicts across multiple versions of the same crate. It’s normal and very common to have multiple versions of the same crate in your dependency tree (have a look at cargo-tree’s output for any reasonably large project) and without this mangling, it would be very difficult to build any large project that had any dependency that used extern "C".

TL;DR: extern "C" doesn't imply no_mangle and there should be no need to manually version those symbols.

@ramosbugs
Copy link
Author

I think I've addressed the comments at this point, other than some questions I posed above

@gj
Copy link

gj commented Nov 25, 2019

Hi @briansmith,

Thank you so much for all of your hard work on this library (and others). Are there any outstanding blockers with this PR that you need help with prior to merging?

@Folyd
Copy link

Folyd commented Dec 13, 2019

Hey @briansmith, let's get this PR merged before 2020, please. We have some downstream crates depend on this great PR. Thanks a lot!

@ramosbugs
Copy link
Author

it's clear that this PR is never going to be merged, and I'm tired of seeing it near the bottom of my open PRs queue, so I'm going to go ahead and close it and try to find a crypto library other than ring to use. many of us are stuck at home to quarantine, so maybe a good opportunity for a community-maintained alternative.

@ramosbugs ramosbugs closed this Apr 1, 2020
@TotalKrill
Copy link

Could not an alternative be to just create a new crate that mirrors this but includes this patch on top of it be an alternative?

@briansmith
Copy link
Owner

@ramosbugs Sorry. I am planning to solve this problem one way or another before 0.17 is released. I have a bunch of changes queued up which should reduce the amount of code touched by this MR, to minimize what I have to review. That involves merging the rest of BoringSSL. I am currently at 98f9694; see https://boringssl.googlesource.com/boringssl/+log?s=9511ca4c036c61df51c2018facdcdbf79ff80151 to see the remaining backlog of that merging work.

@briansmith
Copy link
Owner

Actually, I just take a peak at this to get an idea of how much work it would be. Apparently it did get updated to be much nicer (smaller) since the previous time I reviewed it. It seems like the main remaining issue is that the configuration that is tested in CI/CD is different from the configuration tested outside CI/CD. If we need to add the Go stuff into CI/CD to resolve this then we should do that.

@andrewtj
Copy link
Contributor

@briansmith is this still slated for 0.17?

@briansmith
Copy link
Owner

@andrewtj Yes, I'm planning to address the issue in 0.17.0. This PR was closed so I guess we may have to do it independently of this PR.

@briansmith
Copy link
Owner

Issue #1268 tracks solving the problem that this PR attempted to solve. It seems the approach is pretty similar; see the details in issue #1268.

@briansmith
Copy link
Owner

@ramosbugs Sorry this is so late. Thank you very much for your earlier effort on this. I really do appreciate your effort here. It was unfortunate that my availability didn't line up with your interest in the project.

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

9 participants