docker-archive / go-p9p

A modern, performant 9P library for Go.
Apache License 2.0
206 stars 50 forks source link

Expose max message sizes and check boundaries on Read/Write #29

Closed simonferquel closed 8 years ago

simonferquel commented 8 years ago

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.

codecov-io commented 8 years ago

Current coverage is 15.33% (diff: 0.00%)

Merging #29 into master will decrease coverage by 0.62%

@@             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          

Powered by Codecov. Last update f717cf6...f9c7ab2

stevvooe commented 8 years ago

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.

simonferquel commented 8 years ago

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:

func (c *client) Read(ctx context.Context, fid Fid, p []byte, offset int64) (n int, err error) {

  • if len(p) > c.transport.channel().MaxReadSize() {

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.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

stevvooe commented 8 years ago

@simonferquel Could you discuss on #30?