bluetech / pcsc-rust

Rust bindings to PC/SC for smart card communication
https://docs.rs/pcsc
MIT License
111 stars 23 forks source link

Robustly starting a transaction is unergonomic #25

Closed nwalfield closed 2 years ago

nwalfield commented 2 years ago

First, thanks for working on these bindings for pcsc.

In our project, we're trying to robustly start a transaction. This means having a loop that calls pcsc::Card::transaction, and if that returns pcsc::Error::ResetCard, then calling pcsc::Card::reconnect and retrying. Something like this:

loop {
   match card.transaction() {
    Ok(tx) => return tx,
    Err(pcsc::Error::ResetCard) => {
      card.reconnect()?;
    }
    e @ Err(_) => return e,
  }
}

Since this is a fair amount of code, we want to wrap the complexity in a function (actually, we wrap a lot of functionality and this is just a method).

When we do this in a function (TxClient::new(mut card: PcScCard)), the borrow checker complains:

error[E0597]: `card` does not live long enough
   --> pcsc/src/lib.rs:47:17
    |
34  | impl<'b> TxClient<'b> {
    |      -- lifetime `'b` defined here
...
47  |         let c = card.card();
    |                 ^^^^ borrowed value does not live long enough
...
53  |                 Ok(mut tx) => {
    |                    ------ assignment requires that `card` is borrowed for `'b`
...
118 |     }
    |     - `card` dropped here while still borrowed

error[E0499]: cannot borrow `*c` as mutable more than once at a time
  --> pcsc/src/lib.rs:50:23
   |
34 | impl<'b> TxClient<'b> {
   |      -- lifetime `'b` defined here
...
50 |             let res = c.transaction();
   |                       ^ `*c` was mutably borrowed here in the previous iteration of the loop
...
53 |                 Ok(mut tx) => {
   |                    ------ assignment requires that `*c` is borrowed for `'b`

error[E0499]: cannot borrow `*c` as mutable more than once at a time
  --> pcsc/src/lib.rs:93:25
   |
34 | impl<'b> TxClient<'b> {
   |      -- lifetime `'b` defined here
...
50 |             let res = c.transaction();
   |                       - first mutable borrow occurs here
...
53 |                 Ok(mut tx) => {
   |                    ------ assignment requires that `*c` is borrowed for `'b`
...
93 |                         c.reconnect(
   |                         ^ second mutable borrow occurs here
...

Doing the same thing in a macro, however, works fine.

As far as we can tell, this has to do with reborrowing mutable references. Basically, it is not possible to clone a mutable reference. So rust moves a mutable reference into the called function (e.g., pcsc::Card::transaction). When the result of that function is returned, its lifetime extends beyond the lifetime of the loop, obviously. Thus, the second iteration of the loop is not able to use the mutable reference again. See this explanation for details.

I think a possible solution to the problem would be to have transaction return the mutable reference on error. Thus, there could be a variant of transaction that looks like this:

fn transaction2(&mut self) -> Result<Transaction<'_>, (&mut Self, Error)>

This probably makes the implementation of transaction2 a bit less elegant, but it should help us avoid the reborrowing issue.

What do you think?

nwalfield commented 2 years ago

Using the above MR, which adds the proposed Card::transaction2 method, allows our code to compile, as per the latest commit to https://gitlab.com/nwalfield/openpgp-card/-/tree/reborrow .

bluetech commented 2 years ago

Seems like this is a limitation of Rust: https://github.com/rust-lang/rust/issues/68117. The linked comment suggests it will be fixed with "Polonius" and the following code does indeed work in nightly Rust with the flag -Cpolonius:

fn txn<'card, 'tx>(card: &'card mut Card) -> Result<Transaction<'tx>, Error> where 'card: 'tx {
    loop {
        match card.transaction() {
            Ok(tx) => return Ok(tx),
            Err(pcsc::Error::ResetCard) => {}
            Err(err) => return Err(err),
        }
        card.reconnect(ShareMode::Shared, Protocols::ANY, Disposition::ResetCard)?;
    }
}

however I couldn't come up with a workaround for this in stable Rust (although I'm not really one to know if there were). So I think the PR makes sense.

nwalfield commented 2 years ago

Thanks for taking a look! And I appreciate the tip about -Cpolonius.