Expose max message sizes and check boundaries on Read/Write #29
Conversation
Current coverage is 15.33% (diff: 0.00%)@@ master #29 diff @@
==========================================
Files 13 15 +2
Lines 1128 1174 +46
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 180 180
- Misses 900 946 +46
Partials 48 48
|
…nts an io.Reader and io.Writer handling this limit correctly Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
simonferquel
force-pushed
the
expose_max_message_sizes
branch
from
483d407 to
31dd0e8
Compare
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
| func (c *client) Read(ctx context.Context, fid Fid, p []byte, offset int64) (n int, err error) { | ||
| if len(p) > c.transport.channel().MaxReadSize() { |
There was a problem hiding this comment.
I don't see how read can be handled on the client side. The message flow would look something like this:
- Client requests data of size and offset.
- Server responds with data up to
msize - overhead. - Client returns from this method with
len(resp.Data).
I think we just need to ensure that the server doesn't respond with too large of a message for the channel.
There was a problem hiding this comment.
Problem is, the wording in the man pages seems to indicate that the client should not send message expecting that the response would be longer than msize. What we could do though, instead of failing, is to create a request respecting msize and return the actually read size
There was a problem hiding this comment.
That is basically the right fix. This can mostly be done before the message is sent. Modifying Twrite and Rread inline is okay, since the return value will provide enough information for the sender to know they didn't send all the data.
We still need to handle the server side problem, as well.
We also need a way to set an optimal buffer size but I'd like to see how this is being used. Mostly, I'm worried that something is wrong since it took a year to find such a simple bug. For the most part, we haven't seen these conditions until now. Go's io package can be subtle, so I suspect a problem there.
|
#30 handles the client-side truncation of Twrite. I think there are a few other things we must do to protect the client. Please break out the reader/writer from this PR into a separate PR. |
|
I am just a bit worried about the write counterpart, as from the io.Writer documentation, a write should always return len (data) unless err is not null. Partial Writes do not fit very well with that. On Mon, Oct 24, 2016 at 8:58 PM +0200, "Stephen Day" notifications@github.com wrote: @stevvooe commented on this pull request. In client.go:
That is basically the right fix. This can mostly be done before the message is sent. Modifying Twrite and Rread inline is okay, since the return value will provide enough information for the sender to know they didn't send all the data. We still need to handle the server side problem, as well. We also need a way to set an optimal buffer size but I'd like to see how this is being used. Mostly, I'm worried that something is wrong since it took a year to find such a simple bug. For the most part, we haven't seen these conditions until now. Go's io package can be subtle, so I suspect a problem there. — |
|
@simonferquel Could you discuss on #30? |
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
This implements correct msize handling where Read/Write operations cannot read/write more than msize-[message envelope size] bytes at once.
For easier processing, this behavior is encapsulated in a io.Reader / io.Writer implementation to chunk read and writes when needed.