search

multiformats / go-multistream

an implementation of the multistream protocol in go
MIT License
41 stars 27 forks source link

Possible issue with ls #41

Closed alanshaw closed 5 years ago

alanshaw commented 5 years ago

I'm new to Go, please excuse if this is incorrect!

As far as I can tell, a call to Ls does not read the message length. It reads a varint and assumes it is the number of protocols that are being sent.

i.e. the response for ls\n will be:

<varint-msg-len><varint-num-protos><varint-proto-name-len><proto-name>\n<varint-proto-name-len><proto-name>\n...

But the following function expects it to be:

<varint-num-protos><varint-proto-name-len><proto-name>\n<varint-proto-name-len><proto-name>\n...

i.e. without <varint-msg-len> prefix

https://github.com/multiformats/go-multistream/blob/039807e4901c4b2041f40a0e4aa32d72939608aa/multistream.go#L95-L116

I'm not entirely sure if/how it is working in production. I have a hunch the tests are passing because they're just reading/writing to a buffer and I think they might overwrite the message length prefix with ls\n (needs to be verified!).

Furthermore, I think the Ls handler might be missing a \n somewhere.

I thought it would be most useful if there was a \n at the end of the response i.e. you'd end the message with \n\n, and the response being a single multistream-select message, with embedded messages.

Then you could read the whole response with lpReadBuf (it removes the \n suffix) and then slice off <varint-proto-name-len> and then call lpReadBuf again however many times it is needed on the remainder. The latter is happening but the former is not.

https://github.com/multiformats/go-multistream/blob/039807e4901c4b2041f40a0e4aa32d72939608aa/multistream.go#L336-L361

Note the spec differs considerably https://github.com/multiformats/multistream-select#ls-example-in-detail and wants a \n after <varint-num-protos>. This would work also.

<varint-of-list-of-protos-size-in-bytes> from the spec is not present in this implementation but I think it's superfluous.

I could use a second opinion on whether what I'm interpreting from the code is a bug or not. I can probably then send a PR to fix.

I can also PR the spec, but we should figure out if we should add that \n somewhere (probably a breaking change)...or leave it.

Stebalien commented 5 years ago

You're right, this is totally broken.

I thought it would be most useful if there was a \n at the end of the response i.e. you'd end the message with \n\n, and the response being a single multistream-select message, with embedded messages.

I agree this makes the most sense. I'd even drop the number of protocols, we don't need that. Messages embedded within messages makes the most sense.

If you have a moment to update the spec, I'd say go for it. ls isn't used for anything but debugging anyways.