-
Notifications
You must be signed in to change notification settings - Fork 81
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
Panic triggered when sending file to Wormhole William #136
Comments
And of course, my overly verbose logging still doesn't include the relevant information :( I was in the process of improving things on the logging side anyways, so I'll finish them first before I start investigating this issue. Generally, it looks like the Go side is sending a message that the Rust side is not happy with. Without knowing the specific message that caused the error, either side may be at fault so it was the right thing to post the bug report here. Especially as the Rust code currently may be overly strict in some cases. |
I finally came back to this, and to me this is a bug in the Go implementation. The message they send is:
This is an enum, and as such the fields |
Thank you so much for investigating! Should I pass this off to the developers of Wormhole William then? |
Yes, please. You can link this issue there for further context. |
I will update wormhole-william to not send the additional fields in this message. However, I'm not sure this strict protocol handling in the rust code is ideal. If the rust implementation panics on any unexpected fields in messages that will make it very difficult to add backwards compatible extensions to the protocol in the future. Just something to consider. I've opened psanford/wormhole-william#59 to track the Go change. |
Yes, you are totally right on that. This is tracked in #27. For future proofing, this means that we shall accept additional fields in structs (automatically done by serde) and unknown enum variants (partially implemented). This message is different in the fact that it kind of represents an enum with two variants, and I'd say it is fine to consider this as illegal. But how should we deal with messages like: |
The latest version of Wormhole William on the Google Play store now has this fix. |
When sending any file to the Wormhole William app (android), magic-wormhole.rs panics before the transfer is complete.
Running magic-wormhole installed from source on commit 166d434 (latest), and Wormhole William version 1.0.4 (latest fdroid). Running
wormhole send <filename>
on sending device, and entering the code in WW initially produces an accept/deny prompt, as expected. However, selecting "accept" causes wormhole.rs to panic with the error appended to the bottom of this post.This error does not occur with standard python wormhole, nor with the reverse configuration (ww -> wrs). The error on WW's side is simply "Receive file error: failed to establish connection".
I'm running magic-wormhole.rs on Fedora 35 (although I had this problem on 34 as well), built locally with
cargo 1.56.0 (4ed5d137b 2021-10-04)
Please let me know if there is any other information I can help provide, or if I should move this over to WW's issue tracker instead. I figured I ought to post it here, since panics are rarely expected behavior, even with a misbehaving remote, but I'm happy to move it if it's their client acting up.
Thanks so much for your help and for the awesome app!
(output from running
wormhole --log send <filename>
withRUST_BACKTRACE=1
)The text was updated successfully, but these errors were encountered: