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

Check &self/&mut self #9

Open
jethrogb opened this issue Oct 30, 2018 · 3 comments
Open

Check &self/&mut self #9

jethrogb opened this issue Oct 30, 2018 · 3 comments

Comments

@jethrogb
Copy link
Member

jethrogb commented Oct 30, 2018

There's a lot of *mut pointer usage in the MbedTLS C API but perhaps some of those usages should be *const, especially with regards to callbacks.

@jack-fortanix
Copy link
Contributor

I checked over every instance of &mut self and *mut pointers within mbedtls crate. Sadly I don't see anything to be done - the operations are either actually mutating the object, or they are not but mbedtls requires a non-const pointer for no particular reason. Eg pk_write_key_xxx functions are logically const (and I checked and they actually do not modify the key argument), but still take it by non-const pointer. Ditto ecdh_calc_secret, x509_crt_verify, ... ISTR seeing a bug in mbedtls issue tracker about this, basically they are worried that adding const to these declarations will break some application so are waiting for Mbedtls 3.0

We could start assuming these functions are actually const and then avoid having to take &mut self by instead casting self.inner as *const _ as *mut _, at the risk of nasty breakage if mbedtls instead changes to actually start mutating one of these arguments that they are accepting by mutable pointer. They seem to care about application compatibility so hopefully they wouldn't do that - but nothing in the docs for any of these functions states that the argument is not modified, so maybe as far as they are concerned all bets are off.

@jethrogb
Copy link
Member Author

We could start assuming these functions are actually const and then avoid having to take &mut self by instead casting self.inner as *const _ as *mut _,

Yes that's what I was thinking

@jack-fortanix
Copy link
Contributor

Sounds good to me. Some of these will require some digging to figure out if it is actually safe or not (for instance it appears ecdh_calc_secret may actually modify the context). In Mbed-TLS/mbedtls#1485 (comment) I suggested to upstream that they document the situation better.

bors bot added a commit that referenced this issue Dec 17, 2020
128: MbedTLS Reference counted instead of lifetimes r=jethrogb a=AdrianCX

Moving from referene counting allows simpler move to native-tls / hyper.

Arc Changes:
- Each Config/Context/... will hold Arcs towards items it holds pointers to.
- This forces objects to live as long as needed, once no longer used they get destroyed by reference counting.

This allows passing the objects to multiple threads without worrying about lifetime.
I've also added notes why classes are Sync where used. Let me know if I missed any classes.

Usage example of an intermediate mbed-hyper integration is at: 
- https://github.com/fortanix/rust-mbedtls/tree/acruceru/wip-mbed-hyper-v2/mbedtls-hyper/examples/integrations

There I added a crate to wrap hyper - similar to native-tls. (that will be moved to native-tls layer soon)
That crate can be considered an integration test that I will raise a separate PR for.


Edit:

Changes after initial review:
-    Added forward_mbedtls_calloc / forward_mbedtls_free functions so we can pass certificates to and from mbedtls without allocator mismatches/corruptions.
-    Switched to MbedtlsList<Certificate> and Certificate. A MbedtlsBox is pending for this PR as well.
-    Fixed most comments.

Still pending:
-    Update define! macros
-    Add MbedtlsBox<Certificate>


Fixes #1
Partial progress on #3
Fixes #4
Fixes #8
Partially addresses #9

Co-authored-by: Adrian Cruceru <adrian.cruceru@fortanix.com>
bors bot added a commit that referenced this issue Dec 17, 2020
128: MbedTLS Reference counted instead of lifetimes r=jethrogb a=AdrianCX

Moving from referene counting allows simpler move to native-tls / hyper.

Arc Changes:
- Each Config/Context/... will hold Arcs towards items it holds pointers to.
- This forces objects to live as long as needed, once no longer used they get destroyed by reference counting.

This allows passing the objects to multiple threads without worrying about lifetime.
I've also added notes why classes are Sync where used. Let me know if I missed any classes.

Usage example of an intermediate mbed-hyper integration is at: 
- https://github.com/fortanix/rust-mbedtls/tree/acruceru/wip-mbed-hyper-v2/mbedtls-hyper/examples/integrations

There I added a crate to wrap hyper - similar to native-tls. (that will be moved to native-tls layer soon)
That crate can be considered an integration test that I will raise a separate PR for.


Edit:

Changes after initial review:
-    Added forward_mbedtls_calloc / forward_mbedtls_free functions so we can pass certificates to and from mbedtls without allocator mismatches/corruptions.
-    Switched to MbedtlsList<Certificate> and Certificate. A MbedtlsBox is pending for this PR as well.
-    Fixed most comments.

Still pending:
-    Update define! macros
-    Add MbedtlsBox<Certificate>


Fixes #1
Partial progress on #3
Fixes #4
Fixes #8
Partially addresses #9

Co-authored-by: Adrian Cruceru <adrian.cruceru@fortanix.com>
Co-authored-by: Jethro Beekman <jethro@fortanix.com>
mcr pushed a commit to mcr/rust-mbedtls that referenced this issue Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants