Closed Alch-Emi closed 2 years ago
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:
{"answer":{"message_ack":"","file_ack":"ok"}}
This is an enum, and as such the fields message_ack
and file_ack
are mutually exclusive. I suppose that Go does not distinguish between "empty string" and "no string", where as Rust does. I also assume that this problem will appear on every enum message type (with that encoding), and that it is not specific to this one.
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 https://github.com/psanford/wormhole-william/issues/59 to track the Go change.
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.
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: {"answer":{"message_ack": null,"file_ack":"ok"}}
(i.e. only one non-null field)? I'd say it is reasonable to accept this as valid (as long as the field of the variant itself may never be null), but I'm not sure if our code currently accepts that.
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
)