Upgrade bindgen

[fortanix]

Upgrade bindgen

[nouwaarom]

Hello, I was trying to use this library but had trouble compiling it. Using the changes in #102 I everything compiled and it works fine. What is the reason that issues has been idle for so long? Are there problems with the fixes in #102. In that case I would be glad to help to resolve them.

[nerded1337]

Hi folks. I also tried to use this library on windows, have been able to use it thanks to #102. As @nouwaarom, I also would be glad to help if anything is required to merge this fix.

[ekse]

I would like to give this a try. Since Builder::remove_prefix was removed, are you ok with using names with the "mbedtls_" prefix, except when we reexport values?

[jethrogb]

Oh I forgot about this issue. See rust-lang/rust-bindgen#428 . I'd really like to automate this since I'm not a big fan of having to use the prefixes everywhere.

[ekse]

I worked on this issue in the last few days, here is a status update on my progress so far. I would like to discuss potential approaches to deal with issues I encountered. You can find my work branch at https://github.com/ekse/rust-mbedtls/tree/bindgen-upgrade.

First, a quick note on why I want to upgrade. The current master branch does not compile on my computers, it fails with the error Bindgen ERROR: unsupported type CXTypeKind(119). Upgrading bindgen fixes this issue. I get this build error on libclang 3.9, 4.0 and 6.0.

"mbedtls_" prefix

To deal with the "mbedtls_" prefix, I tried the approach suggested by the bindgen maintainer to rename the imports with use x as y statements. To reduce the amount of boilerplate, I experimented with creating a procedural macro "mbedtls_use" https://github.com/ekse/rust-mbedtls/tree/bindgen-upgrade/mbedtls-use. An example use of this macro looks like this: (https://github.com/ekse/rust-mbedtls/blob/bindgen-upgrade/mbedtls/src/cipher/raw/serde.rs#L24)

#[mbedtls_use]
use {
    mbedtls_aes_context, mbedtls_cipher_context_t, mbedtls_cipher_id_t, mbedtls_cipher_mode_t,
    mbedtls_des3_context, mbedtls_des_context, MBEDTLS_CIPHER_ID_3DES, MBEDTLS_CIPHER_ID_AES,
    MBEDTLS_CIPHER_ID_DES, MBEDTLS_MODE_CBC, MBEDTLS_MODE_CFB, MBEDTLS_MODE_CTR,
};

There are a couple downsides to this approach. Procedural macros were just stabilized in Rust 1.30, that would make it a hard requirement. Also, the use statements are not in scope when using macros like define!, so I used the full type names (see https://github.com/ekse/rust-mbedtls/blob/bindgen-upgrade/mbedtls/src/pk/ec.rs#L19).

Alternatively, we could remove the proc_macro and write the complete use x as y statements, at the cost of longer use statements.

unsigned constants

The second issue I encountered is with constants. Bindgen defines constants such as MBEDTLS_SSL_IS_CLIENT as u32. This fails to compile for define! statements that map to c_int as it maps to i32. I assume the older version of bindgen defined those constants as i32.

error[E0308]: match arms have incompatible types                                                                                            
  --> mbedtls/src/wrapper_macros.rs:75:5                                                                                                    
   |                                                                                                                                        
75 |                   match self {                                                                                                         
   |  _________________^                                                                                                                    
76 | |                     $($n::$rust => ::mbedtls_sys::$c,)*                                                                              
   | |                                    ----------------- match arm with an incompatible type                                             
77 | |                 }                                                                                                                    
   | |_________________^ expected i32, found u32                                                                                            
   |                                                                                                                                        
  ::: mbedtls/src/ssl/config.rs:62:1                                                                                                        
   |                                                                                                                                        
62 | / define!(enum UseSessionTickets -> c_int {                                                                                            
63 | |     Enabled => MBEDTLS_SSL_SESSION_TICKETS_ENABLED,                                                                                  
64 | |     Disabled => MBEDTLS_SSL_SESSION_TICKETS_DISABLED,                                                                                
65 | | });                                                                                                                                  
   | |___- in this macro invocation  

Also, some getters and setters fail to compile because enums do not implement From conversions for i32 (because values in u32 could be outside the range of i32).

setter!(set_session_tickets(u: UseSessionTickets) = mbedtls_ssl_conf_session_tickets)

error[E0277]: the trait bound `i32: std::convert::From<ssl::config::UseSessionTickets>` is not satisfied                                    
   --> mbedtls/src/wrapper_macros.rs:192:50                                                                                                 
    |                                                                                                                                       
192 |             unsafe{::mbedtls_sys::$cfn(&mut self.inner,$n.into())}                                                                    
    |                                                           ^^^^ the trait `std::convert::From<ssl::config::UseSessionTickets>` is not implemented for `i32`

I temporarily worked around this by manually defining the constants as i32, and the crate successfully compiles at that point. To could be an approach to deal with this issue, I'm not sure what the best way to do that with bindgen would be.

---

Sorry for the long message, it seems upgrading the bindgen dependency will require a good amount of effort. I suggest we decide on a way to deal with the mbedtls_ prefix issue and move on from there.

Thanks,

Sébastien

[jethrogb]

prefix

If you take a look at the latest comments at rust-lang/rust-bindgen#428 it seems to me that reimplementing remove_prefix shouldn't be too hard. That would have my preference over procedural macros.

constants

I think it should be pretty easy to replace the current macro_int_types configuration with an appropriate ParseCallbacks::int_macro implementation.

[ekse]

Thanks for the reply, I had not seen the reply on the bindgen issue. I implemented an item_name` callback and it is now merged in the master branch of bindgen.

I pushed a new branch with your suggested approaches, the changeset is now much much smaller.
master...ekse:bindgen-upgrade-v2

client_server_testis failing with X509CertVerifyFailed on the client. I will try to figure out why.

[ekse]

Haha I found it: The certificate is expired, the expiration date is '16 Aug 2016'.

[jethrogb]

@ekse any progress? Do you need help figuring anything out?

[ekse]

I opened a pull request (#13) to replace the expired certificate in the tests.

For the bindgen upgrade, I was waiting for a new release of bindgen. I asked the maintainers today if they could make a new release. My branch currently compiles and all the tests are passing, it depends on the git repository of bindgen at the moment, so once the bindgen crate is updated it will be ready for a pull request.
master...ekse:bindgen-upgrade-v2

[jethrogb]

This is also necessary for the docs to show up properly on docs.rs

[jack-fortanix]

With recent nightly, the version of syntex-syntax pulled in by bindgen 0.19 fails to compile.

In some months, that nightly behavior will become stable at which point mbedtls crate will be broken :(

@jethrogb it seems like #14 is stalled on unresolved bindgen bugs. It is possible we can move to some intermediate version of bindgen to avoid the compilation problem but without hitting the bugs in more recent bindgen?

[jethrogb]

No, we are on the latest possible bindgen. It works if you patch syntex_syntax to 0.40 and disable deny(warnings)

[bsdinis]

I echo the previous requests. This is too much of a problem to be left unfixed (I came here because this breaks fortanix sgx sdk tls example, so you can imagine).

Let me know if there is help required with this.

[jethrogb]

As mentioned in #14 (comment) the new version of bindgen (after 0.19) is not capable of generating the bindings the way we are generating them today, so they'd be incompatible. In order to make progress, features need to be added to bindgen to make sure it can generate code the right way.

[jethrogb]

Final PR: #152