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

Allow a transaction to own the underlying card #28

Open
nwalfield opened this issue Feb 16, 2022 · 6 comments
Open

Allow a transaction to own the underlying card #28

nwalfield opened this issue Feb 16, 2022 · 6 comments

Comments

@nwalfield
Copy link

Imagine that you have a small virtual machine, which is implemented as a state machine. It might be driven by a program like:

open SERIAL_NUMBER
begin-transaction
read-do 0x63
end-transaction

The state machine's state can't easily hold both the card and the transaction, because Rust doesn't like self-referential data structures. To work around this, the transaction could own the card.

nwalfield added a commit to nwalfield/pcsc-rust that referenced this issue Feb 16, 2022
  - Since Rust's support for self-referencial data structures is weak,
    there are cases where it is useful for a transaction to own the card
    and not just for it to hold a mutable reference.  In these cases,
    the caller needs to explicitly end the transaction to recover the
    underlying card, but that is acceptable.

  - Generalize `Transaction` to be generic over a `BorrowMut<Card>`.
    Add new functions `Card::begin_transaction_owned` and
    `Card::begin_transaction2_owned` to create this type of transaction.
    Change `Transaction::end` to return the underlying card.  And, add a
    variant, `Transaction::end2`, to unconditionally return the
    underlying card independent of whether ending the transaction
    succeeded.

  - Fixes bluetech#28.
@nwalfield
Copy link
Author

Note: the provided patch changes the API. As such, it would be necessary to bump the version to 3.0.

@nwalfield
Copy link
Author

For more context, please see: https://gitlab.com/hkos/openpgp-card/-/issues/17

@bluetech
Copy link
Owner

bluetech commented Mar 5, 2022

Hi @nwalfield, I've been thinking about this issue. Let me lay it out chronologically...

  1. I completely understand the problem.
  2. First I tried to think of ways to make it work (with things like Pin and whatnot), but I am not a Rust expert and it's probably too complicated right now.
  3. Then I started to acquiesce to your idea of having an owned transaction variant.
  4. I would like to avoid a breaking change, and the Borrow stuff is again too complicated for my taste, so I think it would be better to just have card.transaction_owned which returns a new type TransactionOwned.
  5. But this is starting to get really inelegant...

So my final thought is: your use case requires sufficient flexibility that the Rust borrow checker is just not worth dealing with. I think, let's keep Transaction as is, for the straightforward lexical-scope cases where the Rust borrow checker provides nice static guarantees, but expose card.begin_transaction()/card.end_transaction() functions (which take &self and don't return anything) for the more complicated cases. PCSC does check that the API is used correctly dynamically so there is unsafety here. We should just make it clear in the docs of these functions that transaction() is a better alternative, when it's possible to use it.

What do you think?

@nwalfield
Copy link
Author

Hi @bluetech,

Thanks for following up.

I like option 4: having pssc::Card::transaction_owned take ownership of the pcsc::Card and return a TransactionOwned would indeed solve the problem and it wouldn't cause an API break. Happily Transaction doesn't expose must functionality so there will be at most minimal redundancy.

Option 6 (your final thought), I find worrying, because it is less type safe. Option 4 means that the API prevents the user from attempting to start a second transaction at compile time, because transaction_owned takes ownership of the pcsc::Card. Option 6's begin_transaction, however, just takes (and presumably doesn't hold) a reference, so that could only be caught at runtime.

I don't think option 4 adds much complexity. If you agree, I'll implement it.

Thanks in advance!

@bluetech
Copy link
Owner

I agree that the static safety provided by Transaction is great. But TransactionOwned which takes ownership of the card feels wrong to me, and is also a bit tricky to use since the user must make sure not to rely on Drop, and Rust currently doesn't support "turning Drop off" (linear types).

I think that option 6 is the best way forward, since it allows users when Transaction is not viable to build their own abstraction. In your state machine for example it seems natural for the state machine to control the dynamic lifetime of the transaction rather than the Rust type itself.

@nwalfield
Copy link
Author

If the owner of the transaction calls drop (implicitly or explicitly) without getting the owned Card back, either it is a bug or intentional. If it is a bug, then the bug will be caught at compile time, because the compiler will complain that the Card has been moved when the developer tries to use the Card again. That's a clear benefit over option 6.

It would make sense to provide option 6, if option 6 is more expressive. I'm having trouble envisioning a case where option 6 can be used to do something useful, which option 4 can't do. In other words, it seems to me that option 4 can do everything that option 6 can, but provides more safety. Do you have something in mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants