Version all C/asm symbols to enable cross-version linking. #619

SergioBenitez · 2018-01-02T21:43:51Z

Resolves #535.

Due to a bug in the current stable Rust compiler (rust-lang/rust#44851), this fix won't work on a stable release until the next stable version. Thankfully this is only a few days away as version 1.23 is scheduled to be released on January 4, 2018.

I've tested this on OS X, Linux (Debian), and Windows (Server 2016) with cargo test and cargo test --features=rsa_signing. I've confirmed that all public symbols exported from compiled binaries are versioned on each of these platforms. The only caveat is that I'm getting a warning somewhere along the chain on Windows that I've yet to track down.

This makes use of a new crate I've written for this purpose; I've named that crate native_versioning for now. For an explanation of how this works, please see that crate's docs. In short:

The general idea is that all symbols in C, C++, and assembly code are mangled with the current crate version. For instance, a symbol some_func will become some_func_v1_23_2_beta. On the Rust side, all symbols will be linked to using the mangled name. The C, C++, and assembly name mangling will happen transparently; only a versioned.h file needs to be maintained, and no changes need to be made to the C, C++, or assembly code. On the Rust side, all non-system extern blocks are modified to use the versioned_extern! macro.

Once the PR is ready to be accepted, I'll publish the native_versioning crate and update the PR with the crate's final name.

briansmith · 2018-01-02T23:57:22Z

build.rs


 fn main() {
+    write_versioned_headers();


How does this work w.r.t. incremental compilation?

How do we ensure all the C/asm code is recompiled when the versioned headers are changed?

How do we ensure that the C/asm code isn't recompiled more than necessary?

Currently, the generated generated_versioned.h and yasm_versioned.h headers are written out each time. Since these aren't tracked in RING_INCLUDES, they shouldn't cause a recompilation. The only header that should cause a recompilation is the manually written versioned.h header. This header is included in RING_INCLUDES, so it should trigger recompilation correctly. Thus, I don't think we need to do anything special here to get the right thing to happen. But if I've misunderstood something, do let me know.

The only other optimization I see is to only regenerate the generated header files if versioned.h is updated.

generated_version.h will change whenever the ring version number is bumped, right? In that case, everything that includes it, directly or indirectly, needs to be recompiled, so that all the symbols get reversioned right? If so then generated_versioned.h. and yasm_versioned would need to be in RING_INCLUDES or we'd need equivalent logic.

Also, I think we need to have logic that builds these generated header files, then compares the contents wit the contents of any existing header files, and only overwrites the file if there is a difference. That way the time stamp of the files won't change spuriously and we wouldn't trigger recompilation of all the C code each time.

So I'm not 100% certain how native_versioning works but shouldn't it be able to just compile the C files normally and then change the symbols before it links them with the Rust code? This wouldn't be as simple as running CC again on the same set of files with some changed, but in theory it should be fine.

@clarcharr Yes, that's also a possible solution. That still requires you to know each symbol you want to change, however, which means you still have to keep a list somewhere. And, what's more, it also requires an objcopy-like utility to be present. Finding/invoking the utility in a cross-platform and cross-target way can be a challenge.

@SergioBenitez you wouldn't need to keep a list; any symbol that's not linked to a dynamic library should be renamed, right?

Yeah, that's true; all public symbols. Still, I think the other concern, requiring an objcopy-like utility, is the real issue, and the main reason why I didn't go this route. Note that such a utility would need to exist on the host for the target. I'm not sure how reasonable a requirement that is. We don't have any library like cc for objcopy, however, so we'd effectively need to write one.

It's a much more complex solution than what we have now. The current solution never recompiles or reads over binaries; it just compiles once with the symbols redefined.

briansmith · 2018-01-02T23:59:01Z

build.rs

@@ -159,7 +162,8 @@ const RING_INCLUDES: &'static [&'static str] =
      "include/GFp/bn.h",
      "include/GFp/cpu.h",
      "include/GFp/mem.h",
-      "include/GFp/type_check.h"];
+      "include/GFp/type_check.h",
+      "include/versioned.h"];


This file should be under a GFp directory so that all the include statements are of the form GFp/versioned.h to prevent conflicts. If that's problematic then we could also just rename the file to have a "gfp_" prefix to its base name.

Should yasm_versioned.h be here too? The same name collision concerns apply to it.

Hmm. Well, we don't know the path to yasm_versioned.h since it's generated at build-time, so we wouldn't be able to add it to this array. Because it's generated, however, I don't think it needs to be included in this array. Please do correct me if I'm wrong, however.

briansmith · 2018-01-03T00:00:52Z

Cargo.toml

@@ -279,6 +276,7 @@ name = "ring"
 [dependencies]
 libc = "0.2.34"
 untrusted = "0.6.1"
+native_versioning = { git = "https://github.com/SergioBenitez/native_versioning" }


In order for us to be able to use this it would need a MIT/ISC/Apache2 license and it would need to be released on crates.io.

Of course. I've added the license to the repository, and as I said in the PR, I'll release it on crates.io once this PR is ready to be merged.

I don't want to use the crate w/o it also being ISC licensed. I don't have any intention of forking it or folding it into ring, but I also want the reassurance that we could fold a variant of it into ring if the dependency were to ever become problematic for any reason, and that would only be realistic if it were ISC licensed since the rest of the ring build is ISC licensed.

I really don't want to get into a license discussion here, but why isn't the MIT license sufficient? It would clearly allow you to do what you're proposing.

I really don't want to get into a license discussion here, but why isn't the MIT license sufficient? It would clearly allow you to do what you're proposing.

I also want to avoid a licensing discussion. ring's build.rs is ISC-licensed and we need to be able to deal with unwanted changes to the native_versioning crate by being able to copy the code into ring using the same license we use for all the Rust code in ring. I don't want to be in the situation where our only reasonable alternative would be to do a new clean-room implementation of the logic.

I think it's rather reasonable to accept the MIT license. But, I really don't care to argue this further, so I've added the ISC license to native_versioning.

briansmith · 2018-01-03T00:05:57Z

build.rs

    let mut c = Command::new("yasm.exe");
    let _ = c.arg("-X").arg("vc")
             .arg("--dformat=cv8")
             .arg(oformat)
             .arg(machine)
+             .arg("-P").arg(&yasm_versioned_symbols_header())


Instead of doing this, I think we can just change the PerlAsm translator so that it adds an #include to the generated asm.

There are a couple of assembly files (x25519-asm-$arch.S) that aren't generated by PerlAsm, so we'd still need to do something about those, too.

OK, thanks for reminding me of that.

briansmith · 2018-01-03T00:06:28Z

build.rs

+        use std::fmt::Write;
+        let line = line?;
+        if line.starts_with("#define") {
+            write!(new_data, "%{}\n", &line[1..]).expect("write to str");


Why is this needed? Notice that none of the *.pl files do this. I think it may be because PerlAsm does this translation itself. See the suggestion below about having PerlAsm write an #include into the generated asm files.

This is for yasm on Windows.

Probably makes sense to add a comment note about that.

briansmith · 2018-01-03T00:08:01Z

build.rs

+    let expanded = cc::Build::new()
+        .file(&intermediate)
+        .include(&generated_include_dir())
+        .expand();


I didn't look at the native_versioning crate's source code yet (since I don't know the license). What is the reason that this has to be run through the preprocessor here?

Yes, this whole dance is so that symbols are versioned by yasm on Windows as well. We can't use the versioned.h header file for yasm because its pre-processor uses different syntax. So, instead of having to maintain two versioned.h files, one for C compilers and another for yasm, I opted to generated a yasm-compatible header file in this function from the existing versioned.h file.

If this is only needed for yasm then we should only run it when we'd run yasm (windows targets).

However, like I mentioned in the other issue, I think that we should get rid of all the #defines in versioned.h and instead distribute those #defines where the declarations/definitions are. This might result in a small number of cases where we need to duplicate the use of VERSIONED() between C and the assembler code, but that would be less problematic than continuously maintaining the versioned.h file as we rename and add symbols.

That was my initial approach but quickly proved to be much easier said than done. It's very easy to miss a symbol this way and doing this for the perl generated assembly files is even more difficult. We'd need to modify each generator individually or keep a table of which symbols each generator generates. Then we also have to handle the yasm case specially.

I don't think maintaining the versioned.h file is any more or less difficult than maintaining #define's in individual files, but I do think that one versioned.h file is an overall cleaner and simpler solution.

That was my initial approach but quickly proved to be much easier said than done. It's very easy to miss a symbol this way and doing this for the perl generated assembly files is even more difficult.

There are two cases to consider: How difficult it is for you to write this PR, and how difficult it is for us to change ring in the future after this PR. I understand that getting everything right in this PR doing it the way I propose is tricky; however, the upside of doing it the way I propose is that we're less likely to accidentally forget to add/change the #define when we add or rename symbols incrementally going forward. In other words, it's going to be somewhat error-prone either way, and we should optimize for minimizing errors in future PRs by people who aren't familiar with this PR.

I think we should go further and just use GFp_VERSIONED() in the C declaration and C definition of all of the functions that are only called from Rust code, instead of relying on these #defines. We really only would need these #defines for functions that are called from the C code, which is smaller and shrinking number of call sites.

For example, instead of:

#define GFp_aes_gcm_open GFp_VERSIONED(GFp_aes_gcm_open) ... int GFp_aes_gcm_open(const void *ctx_buf, uint8_t *out, size_t in_out_len, uint8_t tag_out[EVP_AEAD_AES_GCM_TAG_LEN], const uint8_t nonce[EVP_AEAD_AES_GCM_NONCE_LEN], const uint8_t *in, const uint8_t *ad, size_t ad_len);

We can just have:

int GFp_VERSIONED(GFp_aes_gcm_open)( const void *ctx_buf, uint8_t *out, size_t in_out_len, uint8_t tag_out[EVP_AEAD_AES_GCM_TAG_LEN], const uint8_t nonce[EVP_AEAD_AES_GCM_NONCE_LEN], const uint8_t *in, const uint8_t *ad, size_t ad_len));

Because this function is never called from C code.

It's not just that what you're proposing is harder to implement, it's that it's a really complex solution that is difficult to get right. I feel confident that what I've implemented here is correct. It's a simple solution that's simple to audit. Were I to manually edit each and every C, assembly, and perl file, I would not be confident that I've covered all bases. I think what you're proposing is a time consuming endeavor with little to no benefit.

In coming up with a solution, maintainability was a big concern of mine. With the present solution, upstream changes from BoringSSL can be merged as easily as before since there are zero changes being made to any of the legacy code. With what you're proposing, each change has to be manually adjusted to properly version symbols.

I imagine most pull requests being made to ring are changes to Rust code; this PR doesn't introduce any overhead there except for using versioned_extern! instead of extern. But, if a PR is made that changes C/assembly code, I think it's easy for the reviewer to keep in mind that symbols are versioned. Or, at least, no more or less hard than the solution you're advocating for would make it.

I feel like your primary concern is getting this right. That's mine as well. If that's the case, then I propose the following compromise: it's very simple to write a script that checks that all public symbols in object files produced from C/assembly are properly versioned. I'll write such a script, and we'll run it on Travis. This way, it's not possible for PRs to miss versioning symbols. What do you say?

briansmith · 2018-01-03T00:10:01Z

Thanks for this. Please add the licensing info to the native_versioning crate so I can read its source code to do this review.

coveralls · 2018-01-03T02:35:29Z

Coverage decreased (-0.03%) to 96.156% when pulling 96c46b3 on SergioBenitez:versioned into 6f691f6 on briansmith:master.

coveralls · 2018-01-03T09:46:03Z

Coverage remained the same at 96.19% when pulling d7ba5c2 on SergioBenitez:versioned into 6f691f6 on briansmith:master.

briansmith · 2018-01-04T11:27:49Z

I have a question regarding what happens when we build from crates.io vs when we build from git. What version suffix is appended to symbols when building from a Git checkout? I would expect that either there wouldn't be a version suffix or the version suffix would be the current git commit sha1 or similar.

In particular, consider the case where ring 0.13.0 is released and ring's version number in Git is also still 0.13.0. We need these two copies of ring to have differing version numbers, right?

briansmith

OK, this is looking pretty good overall. Thanks for doing it. However, some things need to be changed.

briansmith · 2018-01-04T10:38:09Z

build.rs

@@ -271,8 +275,79 @@ const MSVC: &'static str = "msvc";
 const MSVC_OBJ_OPT: &'static str = "/Fo";
 const MSVC_OBJ_EXT: &'static str = "obj";

+const GENERATED_VERSIONED_HEADER: &str = "generated_versioned.h";
+const GENERATED_YASM_VERSIONED_HEADER: &str = "generated_yasm_versioned.h";
+const GENERATED_VERSIONED_MACRO: &str = "VERSIONED";


This name, and all other names defined in ring's source code, needs to start with "GFp_" to avoid any potential for collisions.

briansmith · 2018-01-04T10:51:26Z

include/GFp/versioned.h

@@ -0,0 +1,400 @@
+#include <generated_versioned.h>


This file name needs to be qualified with "GFp/" or "gfp_".

briansmith · 2018-01-04T10:59:46Z

include/GFp/versioned.h

+#define gfp_little_endian_bytes_from_scalar VERSIONED(gfp_little_endian_bytes_from_scalar)
+
+// For MacOS symbols generated from perl scripts.
+#define _GFp_AES_encrypt VERSIONED(_GFp_AES_encrypt)


Instead of doing this, can't the header file that defiles VERSIONED handle this? Basically, when we detect that the macOS assembler is being used, add the '_' prefix. Then we avoid all this duplication.

Also, technically this is not correct for C because we cannot legally #define names that start with underscores in C; all such names are reserved.

When we're on macOS, we need both versions. The first will be used by C regardless while the second will be used by the generated assembly code.

We could, however, generate the second set on macOS only. IE, we'd kill all of the _ ones from versioned.h. At build time, check if we're on macOS and emit a new versioned.h with the _ symbols. If we're not on macOS, we'd simply copy the file over.

The first will be used by C regardless while the second will be used by the generated assembly code.

Sure. But we can make it so that GFp_VERSIONED() expands to a name with a leading underscore only when it is included from an assembly file, and expands without the leading underscore all the rest of the time. Then we can completely eliminate the duplication.

No, that's not correct. The #define has to redefine the underscored symbol (_sym) to a versioned underscored symbol (_sym_vxxx) since _sym is what's in the assembly file. We absolutely need something that's effectively:

#define _sym VERSIONED(_sym)

briansmith · 2018-01-04T11:01:07Z

include/GFp/versioned.h

+// For MacOS symbols generated from perl scripts.
+#define _GFp_AES_encrypt VERSIONED(_GFp_AES_encrypt)
+#define _GFp_AES_set_encrypt_key VERSIONED(_GFp_AES_set_encrypt_key)
+#define _GFp_BN_copy VERSIONED(_GFp_BN_copy)


Most of these underscore-prefixed names are actually not necessary, right? GFp_BN_copy(), for example, the weird thing the assembler does isn't relevant since the function is written in C, right?

That's right. I'm being conservative here. If you know of a simple way to determine all symbols that are only used in assembly, I'm happy to pare down the list to those.

If you know of a simple way to determine all symbols that are only used in assembly, I'm happy to pare down the list to those.

That's the advantage of my alternative proposal where we use GFp_VERSIONED() directly within the assembly language files; all of this extra effort for macOS would be automated away.

In a future iteration we could even change the PerlAsm translator so that it knows to do these translations automatically, .e.g. by changing the translation for .extern. Then we'd have even less manual stuff to maintain going forward.

briansmith · 2018-01-04T11:03:10Z

include/GFp/versioned.h

+#define GFp_sha512_block_data_order VERSIONED(GFp_sha512_block_data_order)
+#define GFp_sha512_block_data_order_avx VERSIONED(GFp_sha512_block_data_order_avx)
+#define GFp_sha512_block_data_order_avx2 VERSIONED(GFp_sha512_block_data_order_avx2)
+#define GFp_size_t_align VERSIONED(GFp_size_t_align)


In the case of GFp_size_t_align, et al., they are already defined using a macro, so that macro can just be modified to use VERSIONED() itself, something like this:

#define DEFINE_METRICS(ty) \ OPENSSL_EXPORT uint16_t GFp_VERSIONED(GFp_##ty##_align) = alignof(ty); \ OPENSSL_EXPORT uint16_t GFp_VERSIONED(GFp_##ty##_size) = sizeof(ty);

Sure, but I wanted to modify as little C/assembly code as possible. The current solution requires zero modifications. I think it's a nice goal.

Sure, but I wanted to modify as little C/assembly code as possible. The current solution requires zero modifications. I think it's a nice goal.

Again, I think you're thinking about this in terms of minimizing the complexity of this PR. I am much more interested in minimizing the odds that we'll get a future PR wrong, but minimizing the amount of manual effort required for future PRs.

briansmith · 2018-01-04T11:11:23Z

Cargo.toml

@@ -279,6 +276,7 @@ name = "ring"
 [dependencies]
 libc = "0.2.34"
 untrusted = "0.6.1"
+native_versioning = { git = "https://github.com/SergioBenitez/native_versioning" }


I don't want to use the crate w/o it also being ISC licensed. I don't have any intention of forking it or folding it into ring, but I also want the reassurance that we could fold a variant of it into ring if the dependency were to ever become problematic for any reason, and that would only be realistic if it were ISC licensed since the rest of the ring build is ISC licensed.

briansmith · 2018-01-04T11:16:49Z

build.rs

+    let expanded = cc::Build::new()
+        .file(&intermediate)
+        .include(&generated_include_dir())
+        .expand();


If this is only needed for yasm then we should only run it when we'd run yasm (windows targets).

However, like I mentioned in the other issue, I think that we should get rid of all the #defines in versioned.h and instead distribute those #defines where the declarations/definitions are. This might result in a small number of cases where we need to duplicate the use of VERSIONED() between C and the assembler code, but that would be less problematic than continuously maintaining the versioned.h file as we rename and add symbols.

briansmith · 2018-01-04T11:19:50Z

build.rs

    let mut c = Command::new("yasm.exe");
    let _ = c.arg("-X").arg("vc")
             .arg("--dformat=cv8")
             .arg(oformat)
             .arg(machine)
+             .arg("-P").arg(&yasm_versioned_symbols_header())


OK, thanks for reminding me of that.

briansmith · 2018-01-04T11:20:33Z

include/GFp/versioned.h

@@ -0,0 +1,400 @@
+#include <generated_versioned.h>
+


I think it's going to be unreasonable to maintain this file. Let's move each #define to be right above the declaration (when in a header file) or definition (when there is no declaration in a header file.) That would make it easier for us to keep everything in sync and it will work better with incremental compilation. In particular when we add/rename a GFp_BN symbol, we shouldn't need to recompile any C code other than the bignum code.

In the case of the asm/perlasm-defined code, we may need to duplicate the use of VERSIONED() between the C and asm code, but I am fine with that. Overall it would still be easy to maintain. Especially over time there will be less and less C code calling assembly code (I hope) so less and less duplication needed.

briansmith · 2018-01-04T11:25:05Z

build.rs


 fn main() {
+    write_versioned_headers();


generated_version.h will change whenever the ring version number is bumped, right? In that case, everything that includes it, directly or indirectly, needs to be recompiled, so that all the symbols get reversioned right? If so then generated_versioned.h. and yasm_versioned would need to be in RING_INCLUDES or we'd need equivalent logic.

Also, I think we need to have logic that builds these generated header files, then compares the contents wit the contents of any existing header files, and only overwrites the file if there is a difference. That way the time stamp of the files won't change spuriously and we wouldn't trigger recompilation of all the C code each time.

coveralls · 2018-01-04T16:04:13Z

Coverage remained the same at 96.19% when pulling 5b35161 on SergioBenitez:versioned into 6f691f6 on briansmith:master.

SergioBenitez · 2018-01-05T10:56:25Z

I have a question regarding what happens when we build from crates.io vs when we build from git. What version suffix is appended to symbols when building from a Git checkout? I would expect that either there wouldn't be a version suffix or the version suffix would be the current git commit sha1 or similar.

In particular, consider the case where ring 0.13.0 is released and ring's version number in Git is also still 0.13.0. We need these two copies of ring to have differing version numbers, right?

The version appended is always the crate's version and nothing else, regardless of where the crate is being built from. The version in Cargo.toml should be updated appropriately in the git repository, regardless of this change, to prevent incompatible versions appearing as compatible to cargo. In Rocket, for instance, we use -dev to indicate that this is a development version. The -dev pre-release version is added as soon as a breaking change occurs on the master branch.

Nevertheless, I think it's a good idea to append something like _git_SHORTHASH to the symbols to prevent any issues. Any ideas on how to get the shorthash easily from build.rs? Depending on git2 seems like a no-go. Perhaps we could manually read .git/HEAD and perform the lookup if it's a ref: ? We'd then set an environment variable by printing cargo:rustc-env=FOO=bar for use by the versioned_extern! macro.

Edit: Made the necessary changes to native_versioning. We now get symbols that look like: foo_v0_3_0_alpha_3cc582eb where 3cc582eb is the git shorthash. While experimenting with this, I noticed that Cargo really doesn't like having two versions of the same crate from two different sources. It somehow accepts them when you call cargo run the first time (but not some other commands) but fails on the next go. So it looks like this wouldn't actually be an issue. Still, this is a safe addition.

lholden · 2018-01-08T14:12:36Z

Looking forward to this update. I have a couple crates that I depend on that (directly and indirectly) depend on different versions of ring. Super frustrating because it means that for right now, I have to manually do a lot of work to just get my project (a production application) to compile.

briansmith · 2018-01-08T19:53:18Z

The version in Cargo.toml should be updated appropriately in the git repository, regardless of this change, to prevent incompatible versions appearing as compatible to cargo. In Rocket, for instance, we use -dev to indicate that this is a development version. The -dev pre-release version is added as soon as a breaking change occurs on the master branch.

One of the requirements for this feature (from the issue) is: "Further, we must not need to keep track of whether we made an "ABI-breaking" change in the C/asm code."

In build.rs we already have logic to detect whether we're building from git for other reasons. Let's extend that logic to replace the version suffix with something like "_dev" instead of the version number.

briansmith · 2018-01-08T19:54:29Z

Edit: Made the necessary changes to native_versioning. We now get symbols that look like: foo_v0_3_0_alpha_3cc582eb where 3cc582eb is the git shorthash. While experimenting with this, I noticed that Cargo really doesn't like having two versions of the same crate from two different sources. It somehow accepts them when you call cargo run the first time (but not some other commands) but fails on the next go. So it looks like this wouldn't actually be an issue. Still, this is a safe addition.

Sorry, I didn't see this edit. I'll look at the updates you made to that crate.

SergioBenitez · 2018-01-09T21:05:55Z

@briansmith How are we doing here? Really want to get this in to get Rocket working again.

briansmith · 2018-01-09T21:19:56Z

My plan is to merge all the BoringSSL changes from 2017 into ring first. Then we'll update this PR taking into consideration the BoringSSL changes. That way we can more effectively evaluate how maintainable the change would be, based on how much effort it is to update the PR.

nox · 2018-01-21T09:26:45Z

There are people regularly coming on #rust and who wonder if they didn't start looking at Rust too soon, because they are relying on Hyper or some other high-profile crate and some way or another things don't build anymore because they pull in multiple ring versions.

Even if this patch is a bit cumbersome to maintain, I don't think the ecosystem can afford not merging it.

briansmith · 2018-01-21T19:18:48Z

The current highest priority tasks are, in priority order:

Unbreak the Android CI on Travis CI (Travis CI Android emulator-based tests for Android are broken #603).
Merge BoringSSL into ring.
This.

All three of these are blocking the ring 0.13 release.

briansmith · 2018-01-22T07:29:51Z

In the meantime, see https://twitter.com/BRIAN_____/status/955339612381093888.

(Or something like it). Rocket and jsonwebtoken depend on different versions of `cookie`, and those two versions depend on different versions of `ring`, and `ring` has a policy of not being linked more than once into a binary. You'll get an error like this: ``` error: multiple packages link to native library `ring-asm`, but a native library can be linked only once package `ring v0.11.0` ... which is depended on by `cookie v0.9.2` ... which is depended on by `rocket v0.3.9` ... which is depended on by `changes v0.1.0 (file:///home/coleman/Code/Rust/changes)` links to native library `ring-asm` package `ring v0.12.1` ... which is depended on by `jsonwebtoken v4.0.1` ... which is depended on by `changes v0.1.0 (file:///home/coleman/Code/Rust/changes)` also links to native library `ring-asm` ``` My workaround: fork `jsonwebtoken` and depend on an older `ring` that Rocket transitively depends on, v0.11.0 Refs: briansmith/ring#619 briansmith/ring#535

1wilkens · 2018-05-11T14:26:43Z

Any news on this @briansmith @SergioBenitez?
I noticed android support is not a top-priority currently (#603) and I could not find an issue for the BoringSSL merge.
The rocket + ring situation is still very hairy and currently requires either forking every crate to build with 0.12.1/0.13-alpha (I am at 3 currently) or using very old crate version that use 0.11 like jsonwebtoken 2.0.3 (which was released on 18.06.17(!)).

In addition I think the native_versioning crate could be useful for other libraries that link to C also.
IMO publishing that to crates.io and getting real-world data from the use in ring could greatly benefit the ecosystem in general.

briansmith · 2018-05-12T04:30:04Z

The current highest priority tasks are, in priority order:

Unbreak the Android CI on Travis CI (#603).

I decided to skip unbreaking Android CI for now.

Merge BoringSSL into ring.

The BoringSSL merge was just completed 5 minutes ago.

This.

There are a lot of merge conflicts after the BoringSSL merge that need to be resolved before this can be merged.

All three of these are blocking the ring 0.13 release.

I will release ring 0.13 more-or-less as soon as this PR is merged.

briansmith

The first real CI run can be found here.

This is awesome!

In the aarch64-linux-gnu-gcc output:

GFp_aes_c_encrypt
GFp_aes_hw_decrypt
GFp_aes_c_set_encrypt_key

I believe the above functions just need to be added to the header. They are AArch64-/ARM- specific.

getauxval

This function is provided by libc.

In the i686-unknown-linux-gnu output:

ChaCha20_ssse3

This is 32-bit x86 (i686) specific. It seems I forgot to add the GFp_ prefix to it! Probably because I didn't realize that it is (accidentally) public.I will rename it to _ChaCha20_ssse3 so that it becomes private.

briansmith · 2018-06-04T23:50:59Z

Cargo.toml

@@ -291,6 +289,10 @@ lazy_static = "1.0"
 cc = "1.0.3"
 rayon = "0.9.0"

+[build-dependencies.native_versioning]
+git = "https://github.com/SergioBenitez/native_versioning"
+features = ["build"]


When native_versioning is available on crates.io, let's format this like the others:

[build-dependencies] native_versioning = { version = "whatever", features = ["build"] }

briansmith · 2018-06-14T08:05:30Z

Here is my current plan for this.

Create a branch that merges all the changes in this PR (and the PR for the PR).
Find some way to vendor the native_versioning crate into the ring repo on that branch so we can make progress without putting pressure on a crates.io release for it. Based on our usage, I expect some things in native_versioning to change, and it would be good to give that feedback (in PR form) to the native_versioning crate before it bothers with a release.
Figure out how to check the symbols on Windows.
Clean things up based on my unreasonable whims.
Merge the feature branch into master.
Release ring 0.13.0.

1wilkens · 2018-06-18T13:02:25Z

Rebased again and now all remaining CI errors seems to be unrelated to this PR (most recent CI run) 🎉
@briansmith How should we proceed? You open a branch and I point my PR to it? (I don't think there is a way to create a new branch in a PR)

Keats · 2018-07-19T14:46:48Z

Any other blockers on that now that 0.13 has been released?

briansmith · 2018-07-24T03:52:31Z

I spent some time looking at this this weekend. I noticed that the version header is always generated during every execution of build.rs, and also not everything is recompiled if the version header changes. Instead, the version header needs to be generated only if its contents would change, and whenever it does change then everything needs to be rebuilt.

1wilkens · 2018-08-07T08:09:24Z

Paging @SergioBenitez :)
I could take a stab at this but I did not yet take a look at the internals of native_versioning.

Resolves briansmith#535.

…nux only for now)

…ioned_symbols.py

SergioBenitez · 2018-08-24T07:36:32Z

I'm working on getting this back into shape and incorporating @1wilkens' work. Should be back in tip-top shape later this weekend.

briansmith · 2019-02-06T22:20:43Z

OK, the last time I looked at this a while back, there were some parts that I thought were good and there were some parts that I thought were unreasonable. In particular, the hit to incremental compilation in build.rs isn't really acceptable to me because I am constantly rebuilding ring myself all day long and it really slowed down my work by a notable amount.

Over the last few months, off and on, I spent quite a lot of time replacing C code with Rust code. Partially, I did so to make this issue easier to fix in a way that doesn't impact incremental builds.

In parallel to that, BoringSSL implemented some infrastructure to facilitate something similar to this. I haven't merged that into ring yet, but I think it will help.

Here is what I propose:

Move @1wilkens python script and Travis CI changes into their own PR.
Merge the BoringSSL symbol prefixing changes.
Move the versioned_extern! macro definition, without any of the build.rs logic, to a separate PR, modify it to support the prefixing (not suffixing) scheme implemented by BoringSSL, and change all of ring's extern "C" declarations to use it. Note that in the near future I'm planning to do some other things with #[link_name] attributes on these functions that are ring-specific and which will probably require this macros to be modified in ring-specific ways.
Generate (perhaps using the BoringSSL tools) the static header file with the necessary #defines and check that header into Git. We'll rely on Travis CI (@1wilkens's work) builds to tell us when we need to update this file.
Don't bother trying to avoid collisions for non-crates.io releases. In particular, don't bother trying to incorporate the Git commit hash or anything else into the symbols for development builds. Instead we can just use a static prefix, and change the release automation (mk/package.sh) to check that the prefix is consistent with what we're about to release.

This way, we don't need to do any extra work in build.rs and we can be reasonably confident that we'll solve the problems we're trying to solve.

WDYT?

SergioBenitez · 2019-02-07T03:28:01Z

@briansmith I'm all for it. We should definitely use the new built-in prefixing in BoringSSL.

1wilkens · 2019-02-07T08:05:34Z

@briansmith Seems good!
I think the repo is not really ready for the CI checks without the prefixing in place right? Should I open the PR now or wait until this is merged?

erickt · 2019-02-13T15:58:19Z

For reference, mundane uses the BoringSSL facilities to version symbols. It may be a decent reference on how to get it all setup (cc @joshlf who wrote it).

SergioBenitez · 2019-06-18T22:18:28Z

Closing in favor of the approach in #849 which incorporates the upstream support in BoringSSL for symbol prefixing.

briansmith reviewed Jan 2, 2018

View reviewed changes

briansmith reviewed Jan 3, 2018

View reviewed changes

SergioBenitez mentioned this pull request Jan 4, 2018

Update Ring rwf2/cookie-rs#97

Closed

briansmith requested changes Jan 4, 2018

View reviewed changes

SergioBenitez mentioned this pull request Jan 5, 2018

Build error on nightly-2017-12-21 rwf2/Rocket#513

Closed

lawliet89 mentioned this pull request Feb 14, 2018

ring linking errors lawliet89/rocket_cors#32

Closed

HeroicKatora mentioned this pull request Feb 22, 2018

Actix frontend HeroicKatora/oxide-auth#15

Closed

1 task

andrewtj mentioned this pull request Mar 20, 2018

Adds rng support for Fuchsia #634

Closed

SergioBenitez mentioned this pull request May 5, 2018

Update cookie dependency to 0.10 to avoid ring hell rwf2/Rocket#627

Closed

hayd mentioned this pull request Jun 10, 2018

Support HTTP/2 with rustls actix/actix-web#36

Closed

briansmith reviewed Jun 14, 2018

View reviewed changes

SergioBenitez mentioned this pull request Jul 5, 2018

Can you publish new version on crates.io? rwf2/cookie-rs#105

Closed

Keats mentioned this pull request Jul 15, 2018

Update ring Keats/jsonwebtoken#58

Merged

terry90 mentioned this pull request Aug 13, 2018

Incompatible with Rocket atm Keats/jsonwebtoken#56

Closed

SergioBenitez and others added 6 commits August 23, 2018 17:11

Version all C/asm symbols to enable cross-version linking.

e3b48f5

Resolves briansmith#535.

Rename headers for consistency.

3da3f16

Namespace versioned headers and macro.

f619ce2

Add '#[no_link]' to native_versioning import.

e5582dc

[CI] Add util/check_versioned_symbols.py and execute it in travis (li…

6c3e4b3

…nux only for now)

Add missing symbols on ARM/Aarch64 and ignore getauxval in check_vers…

785f7ad

…ioned_symbols.py

SergioBenitez force-pushed the versioned branch from 5b35161 to 785f7ad Compare August 24, 2018 01:22

Add new symbols.

ece541c

SergioBenitez force-pushed the versioned branch from 3f7e34d to ece541c Compare August 24, 2018 22:30

erickt mentioned this pull request Jan 18, 2019

ETA for a 0.15? rustls/rustls#220

Closed

This was referenced Jun 15, 2019

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

Closed

Version FFI symbols to allow cross-version linking #849

Closed

SergioBenitez closed this Jun 18, 2019

SergioBenitez mentioned this pull request May 1, 2021

Ring does not compile if I use tls while I'm on Apple M1 rwf2/Rocket#1627

Closed

Version all C/asm symbols to enable cross-version linking. #619

Version all C/asm symbols to enable cross-version linking. #619

Conversation

SergioBenitez commented Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jan 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith commented Jan 3, 2018

coveralls commented Jan 3, 2018 • edited Loading

coveralls commented Jan 3, 2018 • edited Loading

briansmith commented Jan 4, 2018

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 4, 2018 • edited Loading

SergioBenitez commented Jan 5, 2018 • edited Loading

lholden commented Jan 8, 2018

briansmith commented Jan 8, 2018

briansmith commented Jan 8, 2018

SergioBenitez commented Jan 9, 2018

briansmith commented Jan 9, 2018

nox commented Jan 21, 2018

briansmith commented Jan 21, 2018

briansmith commented Jan 22, 2018

1wilkens commented May 11, 2018

briansmith commented May 12, 2018

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith commented Jun 14, 2018

1wilkens commented Jun 18, 2018

Keats commented Jul 19, 2018

briansmith commented Jul 24, 2018

1wilkens commented Aug 7, 2018

SergioBenitez commented Jan 2, 2018 •

edited

Loading

SergioBenitez Jan 7, 2018 •

edited

Loading

briansmith Jan 4, 2018 •

edited

Loading

SergioBenitez Jan 3, 2018 •

edited

Loading

SergioBenitez Jan 4, 2018 •

edited

Loading

coveralls commented Jan 3, 2018 •

edited

Loading

coveralls commented Jan 3, 2018 •

edited

Loading

SergioBenitez Jan 4, 2018 •

edited

Loading

coveralls commented Jan 4, 2018 •

edited

Loading

SergioBenitez commented Jan 5, 2018 •

edited

Loading

briansmith commented Feb 6, 2019 •

edited

Loading