Use rust-ece with rc_crypto dynamic backend #1570
Conversation
There was a problem hiding this comment.
Thanks @eoger! This looks good to me from a "the logic seems to have moved to the right places" perspective and is a nice reduction in overhead slash increase in legibility for the push crypto code.
I didn't r+ because it's waiting on the upstream changes, which I don't feel particularly qualified to review.
| @@ -3305,7 +3200,6 @@ dependencies = [ | |||
| "checksum strsim 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a" | |||
| "checksum structopt 0.2.18 (registry+https://github.com/rust-lang/crates.io-index)" = "16c2cdbf9cc375f15d1b4141bc48aeef444806655cd0e904207edc8d68d86ed7" | |||
| "checksum structopt-derive 0.2.18 (registry+https://github.com/rust-lang/crates.io-index)" = "53010261a84b37689f9ed7d395165029f9cc7abb9f56bbfe86bee2597ed25107" | |||
| "checksum subtle 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "2d67a5a62ba6e01cb2192ff309324cb4875d0c451d55fe2319433abe7a05a8ee" | |||
There was a problem hiding this comment.
I like that this appears to have removed quite a few dependencies :-)
| @@ -173,6 +173,7 @@ impl Cryptography for Crypto { | |||
| salt: Option<&str>, | |||
| dh: Option<&str>, | |||
| ) -> error::Result<Decrypted> { | |||
| rc_crypto::ensure_initialized(); | |||
There was a problem hiding this comment.
I'm curious, how did you determine where to add these, and can we tell whether you've added them in all the right places?
There was a problem hiding this comment.
Good question, it's mostly manual process (where are the uses of rust-ece?) combined with test failures. It's not perfect obviously but I think I got all the usages.
There was a problem hiding this comment.
What are the downside risks of forgetting one? (Crash, silent failure, memory unsafety...?)
There was a problem hiding this comment.
You would end up with a panic as we force unwrap the OnceCell in rust-ece, which would be caught by the FFI code and transformed into a regular Kotlin/Swift exception.
|
|
||
| // Can't `impl From<crate::Result<T>> for ece::Result<T>` | ||
| // because we don't own either struct :-( |
There was a problem hiding this comment.
Yay, happy to see this comment become obsolete!
I was inspired by @rfk efforts in #1567 and ended up implementing mozilla/rust-ece#36, this PR is to use these changes.
With this PR applied, OpenSSL should only only be pulled for Linux unit testing (see #1432).