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

transaction: Implement Card::reconnect() wrapper #56

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

kgraefe
Copy link
Contributor

@kgraefe kgraefe commented Dec 9, 2024

This gives us access to Card::reconnect() from within a transaction so that we can check the card status and reset it conditionally with no interference from other processes.

This fixes a race condition I had between two processes where both processes passed the status check resulting in two card resets.

@bluetech
Copy link
Owner

Just to make sure, you tested that reconnect works as expected inside of a transaction, the transaction doesn't end? I mostly expect it not to work.

One hitch with this is that it would technically allow doing tx.transaction(). Can you check what happens in this case? If it's just an error then I guess it's OK.

TBH, I regret using Deref, I now think it would have been better to add explicit wrapper methods... So maybe instead of DerefMut we should add an explicit reconnect method?

@kgraefe
Copy link
Contributor Author

kgraefe commented Dec 13, 2024

Just to make sure, you tested that reconnect works as expected inside of a transaction, the transaction doesn't end? I mostly expect it not to work.

Yes, I have tested it. It works and solves the race condition. In fact, we have been using SCardReconnect() within a transaction for years now with no problem. (I'm converting our C application to Rust.)

That being said, I can only tell for pcscd on Linux, not Windows or MacOS.

One hitch with this is that it would technically allow doing tx.transaction(). Can you check what happens in this case? If it's just an error then I guess it's OK.

Right, I did not think about that. I did some tests:

  • I could create a second transaction and drop it immediately. Thus tx.transaction() does neither return an error nor block forever (which is what I expected). The outer transaction was still usable afterwards. However, it feels like an implementation detail of pcscd and might be different on other platforms or even with other versions of pcscd.
  • In all other cases the borrow checker complained at compile time because we're having two mutable references to the same card at the same time.

TBH, I regret using Deref, I now think it would have been better to add explicit wrapper methods... So maybe instead of DerefMut we should add an explicit reconnect method?

I agree.

@bluetech
Copy link
Owner

OK, makes sense. So let's add a reconnect method on Transaction instead of DerefMut.

This gives us access to Card::reconnect() from within a transaction so
that we can check the card status and reset it conditionally with no
interference from other processes.

This fixes a race condition I had between two processes where both
processes passed the status check resulting in two card resets.

Signed-off-by: Konrad Vité <[email protected]>
@kgraefe kgraefe force-pushed the transaction-derefmut branch from b0f43e3 to 9883932 Compare December 13, 2024 10:20
@kgraefe kgraefe marked this pull request as draft December 13, 2024 10:20
@kgraefe kgraefe changed the title transaction: Implement DerefMut<Target=Card> transaction: Implement Card::reconnect() wrapper Dec 13, 2024
@kgraefe kgraefe marked this pull request as ready for review December 13, 2024 14:04
@bluetech bluetech merged commit 3d0dd9b into bluetech:master Dec 13, 2024
6 checks passed
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

Successfully merging this pull request may close these issues.

2 participants