BUGFIX: 0-length strings are valid on the protocol #1
Conversation
OSC v1.0 spesification doesn't demand that the strings aren't empty, thus a 0-lenght string is completely valid argument and commonly used in real world.
|
Upon further testing this still seems to have problems on stopping the decoding on the first encountered empty string |
The padding calculation is bit iffy...
|
The padding calculation seems to cause some errors with 0 length strings. For now I have dealt with this as a special case but some care should be applied to see if this is a real error in the padding calculations that applies to all strings |
|
Regarding the OSC Spec (emphasis mine):
It would seem from this that an OSC String must not be null. But then again, there isn't really a good way to otherwise express an empty string. Regarding the fix, the second change shouldn't be required (I don't think). The padding calculation should give 3 bytes of padding on a null string (i.e. |
It should return an error then. |
|
Well, in programming even a 0-lenght sequence is a sequence.. using go-osc
as the client and verifying that other osc implementations, like nodes osc
package and protokol are decoding the strings correctly leads me to
believe that the padding calculation here is the one that is faulty.
Without the padding change go-osc doens't parse the packet correctly and
loses all arguments following the 0-lenght string.
|
|
Also looking at the packets in byte level the incoming strings are
correctly padded to 4 bytes of nulls.
Returning errors on decoding errors, and even more so untyped errros,
makes running the actual server Hard, as you certainly don't want your
application to panic on illegal packets and differentating fatal and
non-fatal errors on current implementation of ListenAndServe() is really
hard due to poor error hygiene.
Also the ListenAndServe will close the connection on the error and thus
lead to clients receiving errors about closed ports if talking to a given
destination directly instead of broadcast, which isn't desirable.
|
|
Upon further investigation it seems that the key difference is that the messages are sent as part of a osc bundle, without the length altering code non-bundle empty strings are still decoded properly, but bundles fail. This is output from a debug print in my osc-dispatching code, without the mangling of padding length on 0-lenght strings: And with the mangling. The /clock/state is a legacy message send alone, the others are the new state api inside a osc bundle. All messages are generated and decoded on the same version of go-osc. Decoding of the bundles by npm osc package and the protokol osc monitor for osx match the case with the mangling in place. |
|
And this is the output of the messages from Protokol: |
|
interesting. Can you send me a that whole packet raw? I would like to test against it. Regarding issues with |
|
I pushed a few small fixes (from a few weeks ago, that I just committed now), let me know if those help at all. |
|
https://kissa.depili.fi/tmp/osc_packet_captures.pcap The sending code is from https://gitlab.com/Depili/clock-8001 a open source stage timer |
|
One thing I'm noticing from your packet captures, the bundle seems to include more data than your MTU allows for (which appears to be about 1500), cause the last few messages to get dropped consistently. In other news, when I remove those bytes, every thing works fine in my v1 rewrite branch (which hasn't been pushed yet), so that's a good thing. |
|
Hmm, the tcpdump actually chops the packets up due to their length being larger than the reported MTU But the OSC libraries, including go-osc, as seen from above, actually receive all of that data and are happy to parse it all (given the patch to go-osc). Nothing in the OSC spec seems to indicate that a single message must fit into one udp packet on the transport layer. |
|
https://opensoundcontrol.stanford.edu/files/osc-best-practices-final.pdf actually just plainly says that fragmentation is ok, but this also leads to the question why the bundle packets (send by go-osc) don't seem to get fragmentated even when exceeding the MTU |
|
https://kissa.depili.fi/osc_all.pcap error was in the tcpdump filters for not capturing also the fragment pieces. The fragments get reassembled by the kernel automatically for the application. |
That I know, it's just hard to test when I'm exporting raw data from the pcap.
That is interesting, it's possible Go explicitly doesn't care (which I should probably look into, that doesn't seem like a good thing). In other news, I've decided to do my rewrite this week, I'm just wrapping up the actual encoding and decoding bits (fixed a number of bugs, I believe). I'm going to get started on the whole server bit either tomorrow or the next day. If there is anything you'd like added or changed in the API (the coming v1 release is gonna be almost entirely breaking changes), let me know, maybe I can work it in. |
|
The issue with the packets was completely on wrong tcpdump filters, that weren't catching the additional fragments sent. For the server; It would help if the errors returned would be custom ones for decoding so they can be easily processed. Also if the error processing flow wouldn't close the port (and returning to ListenAndServe would reuse the already open port) it would prevent potential packet loss and errors on the client side while the port is closed. Would also like to have a way to shut the server down if needed for graceful shutdown (and in my case reloading of the configuration with potential osc port changes) |
|
I've just pushed a bunch of changes to the $ go get github.com/chabad360/go-osc@v1-serverThere is a good number of API changes and there are a lot more to come. EDIT: I should note, the dispatcher API has a rather large bug in the matching code, I've fixed it locally, but I don't currently have an internet connection. |
|
Just pushed the fix to the dispatcher. |
|
From quick tests with the head of the v1-server branch (5d55194)
everything seems to work after I figured out the necessery changes due to
new api.
|
|
All right, at this point the rewrite should be ready to use (checkout the I'm going to close this PR and the other one in favor of #3. |
OSC v1.0 spesification doesn't demand that the strings aren't empty, thus a 0-lenght string is completely valid argument and commonly used in real world.