Wrong message type is passed from device

vladimirvolek commented 6 years ago

How to reproduce:

enable a passphrase on your device
go to https://szymonlesisz.github.io/trezor-connect-explorer/#/getaddress
click on 'GET ADDRESS' button (popup will show up on a screen + device will show confirmation "Where to enter your passphrase" Device / Host)
close the popup and wait for a second
click on 'Host' on the device
click 'GET ADDRESS' button again

You should see

The current message from the device should be 'Features' not 'PassphraseRequest'. Maybe is cached?

console log:

prusnak commented 5 years ago

@vladimirvolek is this still a thing?

vladimirvolek commented 5 years ago

@prusnak @tsusanka Yes. I just reproduced this issue.

matejcik commented 5 years ago

what I'm somewhat sure is happening here is this:

connect calls the bridge and waits for answer
by closing the pop-up, you cancel the wait, but Trezor is still in state "ready to send reply"
when you select something on the device, the reply is sent out
bridge's next attempt to read USB will return that reply - even though it's happened after a different write.

I'm not sure what's the appropriate place to resolve this. We might want bridge to clear the queue when reconnecting to the device. Or we might want Connect to send a Cancel when the pop-up is closed (i don't know if that's possible though). Or maybe bridge should send the Cancel when a wait is interrupted? Or it could poll the USB descriptor even when nobody is reading, to catch stray messages like this one.

Not much we can do on the firmware side though. As far as the device knows, nothing is wrong. You can't take back the USB message that you already sent.

jpochyla commented 5 years ago

Or it could poll the USB descriptor even when nobody is reading, to catch stray messages like this one.

This is an interesting idea! cc @karel-3d

karelbilek commented 5 years ago

That would probably break the "post" and "read" messages that's implemented for debug link

On Thu, Mar 28, 2019, 1:26 AM Jan Pochyla notifications@github.com wrote:

Or it could poll the USB descriptor even when nobody is reading, to catch stray messages like this one.

This is an interesting idea! cc @karel-3d https://github.com/karel-3d

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/trezor/trezor-core/issues/295#issuecomment-477292950, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGZ8aSXRNXc1BmkZ6EqBNXipeA45Z8-ks5va7frgaJpZM4Vtx6b .

karelbilek commented 5 years ago

bridge can maybe wait for the stray messages in the case "call" call (not separate "post" and "read" call) is interrupted between the input and output part

but this waiting has to be cancelled when another call then posts something to the device

(I don't currently have any free time for playing with bridge. :/ )

On Thu, Mar 28, 2019, 1:26 AM Jan Pochyla notifications@github.com wrote:

Or it could poll the USB descriptor even when nobody is reading, to catch stray messages like this one.

This is an interesting idea! cc @karel-3d https://github.com/karel-3d

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/trezor/trezor-core/issues/295#issuecomment-477292950, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGZ8aSXRNXc1BmkZ6EqBNXipeA45Z8-ks5va7frgaJpZM4Vtx6b .

tsusanka commented 5 years ago

I dare to say this is not a firmware issue, I'm suggesting to move this to trezord-go then. @prusnak could you move it? I don't think I have permissions for that.

prusnak commented 5 years ago

Transferred

karelbilek commented 5 years ago

Does the explorer work with UDP emulator? I can't see anything there.

....oh that's because it's on github.io and trezord has whitelisted origins for trezor.io.

Maybe it would be a good idea to add an option for whitelisting more origins? (https://github.com/trezor/trezord-go/issues/155)

karelbilek commented 5 years ago

...well adding github.io to the allowed websites didn't help. I still see "no connected devices" in the explorer.

karelbilek commented 5 years ago

And clicking on "get address" gets me this (forever loading)

in the github.io site, I see the error

"postMessage: popupMessagePort not found"

karelbilek commented 5 years ago

I got the same issue with the device itself. Tested FFX and Chrome, wallet works (through bridge), not sure where is the issue.

karelbilek commented 5 years ago

Oh, I needed to manually remove #loading from the popup URL and it started working

karelbilek commented 5 years ago

When I played with it, it is not easy to implement, because. On cancelling of the call, we automatically "release" the device, which automatically closes the device on USB level.

So now, we will need to periodically, when cancelled during waiting for call read, do

open the device (on USB level)
read from the device, with a short timeout
if nothing is read, keep this in a set of "devices, that we periodically check"
if something is read, just read all
when device is disconnected or another call is done on the device, remove it from the set of "devices that we periodically check" - and be sure it is done immediately, so the periodic check that throws away the message is not eating message for someone else
and work around all the mutexes that we have everywhere to prevent all the weird bugs

karelbilek commented 5 years ago

Fixed in https://github.com/trezor/trezord-go/pull/156 but I am not sure it's worth the added complexity (and probably some new bugs in the new code).

tsusanka commented 4 years ago

Note that Connect (therefore Suite) now has a workaround for this (see https://github.com/trezor/connect/pull/561 as linked).

trezor / trezord-go

Wrong message type is passed from device #154