channel: truncate twrite messages based on msize #30

stevvooe · 2016-10-25T02:56:15Z

While there are a few problems around handling of msize, the easiest to
address and, arguably, the most problematic is that of Twrite. We now
truncate Twrite.Data to the correct length if it will overflow the msize
limit negotiated on the session.

Other problems with Twrite/Rread are documented in TODOs here, along
with possible solutions.

Signed-off-by: Stephen J Day stephen.day@docker.com

codecov-io · 2016-10-25T02:57:47Z

Current coverage is 15.65% (diff: 0.00%)

Merging #30 into master will decrease coverage by 0.30%

@@             master        #30   diff @@
==========================================
  Files            13         13          
  Lines          1128       1150    +22   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            180        180          
- Misses          900        922    +22   
  Partials         48         48

Powered by Codecov. Last update f717cf6...7725b53

simonferquel · 2016-10-25T09:54:34Z

For the Write operation, LGTM.
However it does not fix the problem for reading large values (we know by experimentation that there are sloppy servers out there)

stevvooe · 2016-10-25T17:01:46Z

@simonferquel let's take it one fix at a time.

I placed some todos in there for the read side fixes. Feel free to take those on but it would be good to get a proper bug report characterizing the problem messages.

stevvooe · 2016-10-26T00:50:30Z

From the io.Writer documentation:

Write writes len(p) bytes from p to the underlying data stream. It returns the number of bytes written from p (0 <= n <= len(p)) and any error encountered that caused the write to stop early. Write must return a non-nil error if it returns n < len(p). Write must not modify the slice data, even temporarily.

Basically, if we truncate, we need to pass through an error. We can still allow the write to occur. I am not sure if we want to rely on the good behavior of writers to ensure this is done correctly.

justincormack · 2016-10-26T10:55:47Z

channel.go

+}
+
+type overflowErr struct {
+	size int //


missing comment?

justincormack · 2016-10-26T10:59:06Z

The io package defines ErrShortWrite (also ErrShortBuffer) see https://golang.org/pkg/io/#pkg-variables for this case.

stevvooe · 2016-10-26T19:24:22Z

The io package defines ErrShortWrite (also ErrShortBuffer) see https://golang.org/pkg/io/#pkg-variables for this case.

@justincormack Do you think it is sufficient to return ErrShortWrite then have the caller adjust the buffer size to the returned n (bytes written)?

stevvooe · 2016-10-26T20:31:49Z

@justincormack I've updated the PR accordingly.

simonferquel · 2016-11-10T09:01:16Z

I think ErrShortWrite is a good option. We can also provide a io.Writer wrapper that handles the chunking according to ErrShortWrite (so the caller won't have to compute the correct offset for the next chunk).

Reads can be fixed in the same fashion, except that io.Reader does not specify that a short read should return a non-nil err. However, we have to make sure we return a correct io.EOF error at some point such that io.ReadFull works correctly even if len(buf) > [length of the actual 9p file - initial offset] with the provided io.Reader wrapper.

@stevvooe @justincormack does it make sense to you ?

simonferquel · 2016-11-10T09:06:38Z

io.Reader / io.Writer wrappers could be created by functions like:

CreateReaderWriter(session *Session, ctx context.Context, fid Fid, offset int64) *io.ReaderWriter

stevvooe · 2016-11-10T20:55:31Z

However, we have to make sure we return a correct io.EOF error at some point such that io.ReadFull works correctly even if len(buf) > [length of the actual 9p file - initial offset] with the provided io.Reader wrapper.

This is a basic requirement of an io.Reader.

CreateReaderWriter(session *Session, ctx context.Context, fid Fid, offset int64) *io.ReaderWriter

Typically, I would avoid creating a duplex stream, unless you're trying to implement net.Conn.

Also, never use pointers to interfaces.

Have you tested this? Can we merge?

simonferquel · 2016-11-14T10:41:41Z

channel.go

+		// The truncation is reflected in the return message (Rwrite) by
+		// the server, so we don't need a return value or error condition
+		// to communicate it.
+		msg.Data = msg.Data[:len(msg.Data)-overflow]


msg seems to be a copy of fcall.Message (I launched test from #31) on this branch with debugging.
adding fcall.Message = msg fixes the issue

Should be addressed.

stevvooe · 2016-11-15T03:09:17Z

@simonferquel This PR should now handle the covered issues, from client and server.

simonferquel

Almost good to merge to me. Will begin work on datakit client today

simonferquel · 2016-11-15T08:56:29Z

channel.go

+
+		// first, craft the shape of the response message
+		resp := newFcall(fcall.Tag, MessageRread{})
+		overflow := uint32(ch.msgmsize(resp)) + msg.Count - uint32(ch.msize)


We might want to compute empty MessageRRead fcall size once at channel initialization to avoid allocating a new fcall each time we do a read transaction.

Let's take that optimization in a separate PR, with a benchmark.

There is an across the board improvement if we add a field cache to the codec.

simonferquel · 2016-11-15T09:02:41Z

channel_test.go

+func TestWriteOverflow(t *testing.T) {
+	const (
+		msize         = 512
+		overflowMSize = msize * 3 / 2


I'd like to cover the case where len(p) fits in msize, but len(p) + [size of envelope] don't to the regression suite. That would avoid some potential naïve (and wrong) optimizations.

simonferquel · 2016-11-15T09:04:17Z

channel_test.go

+func TestTreadRewrite(t *testing.T) {
+	const (
+		msize         = 256
+		overflowMSize = msize + 1


Same as with writes : I'd like to cover the case where len(p) fits in msize, but len(p) + [size of envelope] don't to the regression suite. That would avoid some potential naïve (and wrong) optimizations.

So, I've clean up these tests. You're welcome to refactoring this one according to the others and create another PR for this one.

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

stevvooe · 2016-11-16T00:10:27Z

Ok, so the tests have been beefed up quite a bit. If there is anything else needed here, please submit a PR.

Let's get this one merged.

While there are a few problems around handling of msize, the easiest to address and, arguably, the most problematic is that of Twrite. We now truncate Twrite.Data to the correct length if it will overflow the msize limit negotiated on the session. ErrShortWrite is returned by the `Session.Write` method if written data is truncated. In addition, we now reject incoming messages from `ReadFcall` that overflow the msize. Such messages are probably terminal in practice, but can be detected with the `Overflow` function. Tread is also handled accordingly, such that the Count field will be rewritten such that the response doesn't overflow the msize. Signed-off-by: Stephen J Day <stephen.day@docker.com>

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

GordonTheTurtle added the status/0-triage label Oct 25, 2016

stevvooe force-pushed the truncate-twrite-msize branch from 35052e7 to d3afddf Compare October 25, 2016 02:58

stevvooe mentioned this pull request Oct 25, 2016

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

Closed

justincormack reviewed Oct 26, 2016

View reviewed changes

channel.go

}

type overflowErr struct {

size int //

Copy link

Contributor

justincormack Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing comment?

stevvooe force-pushed the truncate-twrite-msize branch from d3afddf to 9c7316b Compare October 26, 2016 20:30

stevvooe force-pushed the truncate-twrite-msize branch from 9c7316b to 7725b53 Compare October 26, 2016 20:32

talex5 mentioned this pull request Nov 11, 2016

Go-datakit handle large values when reading / writing in 9db moby/datakit#292

Merged

simonferquel mentioned this pull request Nov 14, 2016

Write overflow - test validating truncated writes #31

Closed

simonferquel reviewed Nov 14, 2016

View reviewed changes

stevvooe force-pushed the truncate-twrite-msize branch 4 times, most recently from 4e1d23f to a8d27f2 Compare November 15, 2016 01:02

stevvooe mentioned this pull request Nov 15, 2016

Added test for overflows on read operation #32

Closed

stevvooe force-pushed the truncate-twrite-msize branch from a8d27f2 to c726d66 Compare November 15, 2016 03:01

simonferquel suggested changes Nov 15, 2016

View reviewed changes

simonferquel added a commit to simonferquel/datakit that referenced this pull request Nov 15, 2016

Adaptation to work with go-p9p docker-archive/go-p9p#30

ed1051e

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel added a commit to simonferquel/datakit that referenced this pull request Nov 15, 2016

Adaptation to work with go-p9p docker-archive/go-p9p#30

c81f5ca

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

stevvooe force-pushed the truncate-twrite-msize branch from c726d66 to a7efa12 Compare November 16, 2016 00:09

stevvooe force-pushed the truncate-twrite-msize branch from a7efa12 to 0f5f58b Compare November 16, 2016 00:47

stevvooe merged commit c74282f into docker-archive:master Nov 16, 2016

stevvooe deleted the truncate-twrite-msize branch November 16, 2016 01:05

stevvooe mentioned this pull request Nov 16, 2016

Read never returns io.EOF / io.UnexpectedEOF #35

Open

djs55 pushed a commit to djs55/datakit that referenced this pull request Mar 16, 2017

Adaptation to work with go-p9p docker-archive/go-p9p#30

d64a17e

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel added a commit to simonferquel/datakit that referenced this pull request Jun 16, 2017

Adaptation to work with go-p9p docker-archive/go-p9p#30

96aec23

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channel: truncate twrite messages based on msize #30

channel: truncate twrite messages based on msize #30

stevvooe commented Oct 25, 2016

codecov-io commented Oct 25, 2016 •

edited

Loading

simonferquel commented Oct 25, 2016

stevvooe commented Oct 25, 2016

stevvooe commented Oct 26, 2016

justincormack Oct 26, 2016

justincormack commented Oct 26, 2016

stevvooe commented Oct 26, 2016

stevvooe commented Oct 26, 2016

simonferquel commented Nov 10, 2016

simonferquel commented Nov 10, 2016

stevvooe commented Nov 10, 2016

simonferquel Nov 14, 2016

stevvooe Nov 15, 2016

stevvooe commented Nov 15, 2016

simonferquel left a comment

simonferquel Nov 15, 2016

stevvooe Nov 15, 2016

simonferquel Nov 15, 2016

simonferquel Nov 15, 2016

stevvooe Nov 16, 2016

stevvooe commented Nov 16, 2016

channel: truncate twrite messages based on msize #30

channel: truncate twrite messages based on msize #30

Conversation

stevvooe commented Oct 25, 2016

codecov-io commented Oct 25, 2016 • edited Loading

Current coverage is 15.65% (diff: 0.00%)

simonferquel commented Oct 25, 2016

stevvooe commented Oct 25, 2016

stevvooe commented Oct 26, 2016

Choose a reason for hiding this comment

justincormack commented Oct 26, 2016

stevvooe commented Oct 26, 2016

stevvooe commented Oct 26, 2016

simonferquel commented Nov 10, 2016

simonferquel commented Nov 10, 2016

stevvooe commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Nov 15, 2016

simonferquel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Nov 16, 2016

codecov-io commented Oct 25, 2016 •

edited

Loading