Support reading multiple bytes in escapeProxy by bamarni · Pull Request #39728 · moby/moby

bamarni · 2019-08-12T18:35:20Z

Currently, the escapeProxy works under the assumption that the
underlying reader will always return 1 byte at a time. Even though this
is usually true, it is not always the case, for example when using a pty
and writing multiple bytes to the master before flushing it.

In such cases the proxy reader doesn't work properly. For example with
an escape sequence being ctrl-p,ctrl-q, when the underlying reader
returns ctrl-p,ctrl-q at once, the escape sequence isn't detected.

A second issue occurs when there is a partial escape currently being
read, if on the next read the len of the passed buffer in argument is
not big enough, it will cause a panic. For example, when the escape sequence
is ctrl-p,ctrl-q, and the caller is passing a []byte of len 1 as argument to read.
And they press ctrl-p, then something else than ctrl-q.

This updates the reader to support this use-cases and adds unit tests.

I'd still like to test more cases to make sure it's working fine, but I'm first
opening the PR to see if it is an acceptable patch.

thaJeztah

Thanks! Did a cursory review, and left some comments/suggestions

thaJeztah · 2019-08-27T13:18:24Z

pkg/term/proxy.go

Wondering if it'd be clearer to return early here (possibly at the cost of some slight duplication)

Here I think it could be replaced with:

n = i + 1 - r.escapeKeyPos if n < 0 { n = 0 } return n, EscapeError{}

Let me know if that looks better and I'll try it out.

yes please! +1 to "return early"

may be in a follow up PR is fine.

thaJeztah · 2019-08-27T13:19:17Z

pkg/term/proxy.go

Not a huge fan of named output variables; it's easy to inadvertently mask them and introduce bugs; perhaps we can slightly refactor the code to not require them

I'd have to make variable declarations at the beginning of the function, this wouldn't look better IMO, I don't see an alternative with the current implementation.

Hmm, 🤞 that we won't have to touch this again! so ok for named output variable here

pkg/term/proxy_test.go

thaJeztah · 2019-09-06T21:29:12Z

Sorry for the delay; #39800 was merged, so you can rebase this PR 🤗

bamarni · 2019-09-07T21:09:28Z

@thaJeztah : awesome 🎉 it's rebased now!

bamarni · 2019-09-08T14:54:58Z

The build failed:

[2019-09-07T21:47:09.271Z] --- FAIL: TestLogBlocking (0.04s)
[2019-09-07T21:47:09.271Z]     cloudwatchlogs_test.go:313: Expected to be able to read from stream.messages but was unable to
[2019-09-07T21:47:09.271Z] time="2019-09-07T21:47:06Z" level=error msg=Error
[2019-09-07T21:47:09.271Z] time="2019-09-07T21:47:06Z" level=error msg="Failed to put log events" errorCode=InvalidSequenceTokenException logGroupName=groupName logStreamName=streamName message="use token token" origError="<nil>"
[2019-09-07T21:47:09.271Z] time="2019-09-07T21:47:06Z" level=error msg="Failed to put log events" errorCode=DataAlreadyAcceptedException logGroupName=groupName logStreamName=streamName message="use token token" origError="<nil>"
[2019-09-07T21:47:09.272Z] time="2019-09-07T21:47:06Z" level=info msg="Data already accepted, ignoring error" errorCode=DataAlreadyAcceptedException logGroupName=groupName logStreamName=streamName message="use token token"
[2019-09-07T21:47:09.272Z] FAIL

See https://ci.docker.com/public/blue/organizations/jenkins/moby/detail/PR-39728/4/pipeline

I'll try to re-run.

bamarni · 2019-09-17T13:44:41Z

@thaJeztah : I've rebased since #39887 was merged and CI passed 🚀

bamarni · 2019-09-26T14:43:26Z

@thaJeztah @runcom : I've rebased with the PR improving the tests and CI is now passing, is there anything else missing here?

bamarni · 2019-10-23T09:52:05Z

@thaJeztah : is this PR still on your radar?

thaJeztah

ah, whoops, no it wasn't 😅

LGTM

thaJeztah · 2019-10-23T12:52:47Z

ping @vdemeester PTAL, I think you're familiar with this area

bamarni · 2019-11-04T12:49:45Z

@vdemeester any news on this one? 👼

bamarni · 2020-03-11T06:26:55Z

Hey @thaJeztah @vdemeester, can you help with this one? It's been sitting here for a while.

dims · 2020-03-12T12:32:05Z

pkg/term/proxy_test.go

thanks for the additional tests. helps understand the problem better.

dims · 2020-03-12T12:33:24Z

LGTM

thanks @bamarni

bamarni · 2020-03-17T06:20:13Z

@dims : so is it good to go, or should I apply your comment about early return here?

dims · 2020-03-17T11:14:03Z

@bamarni if you can quickly make the change for the early return, that would be good. I am fine if you want to do it in a follow up PR as this has been stuck forever

cpuguy83 · 2020-03-17T18:35:17Z

If we are going to make this change, I'd like to simplify this down.
I feel like we can do this with a bufio.Reader and ReadByte.

I kinda started messing with this, it's not 100% yet, but PTAL:

func (r *escapeProxy) Read(p []byte) (nr int, err error) {
	if len(r.escapeKeys) == 0 {
		// This causes tests to fail b/c they are expecting an error in this case.
		// I'm not sure that's correct... but in any case this is a place holder to prevent test panics
		return r.r.Read(p)
	}

	for i := 0; i < len(p); i++ {
		b, err := r.r.ReadByte()
		if err != nil {
			return nr, err
		}
		p[nr] = b

		if b != r.escapeKeys[r.escapeKeyPos] {
			r.escapeKeyPos = 0
			nr++
			continue
		}

		r.escapeKeyPos++
		if r.escapeKeyPos == len(r.escapeKeys) {
			return nr, EscapeError{}
		}
	}

	return nr, nil
}

Currently, the escapeProxy works under the assumption that the underlying reader will always return 1 byte at a time. Even though this is usually true, it is not always the case, for example when using a pty and writing multiple bytes to the master before flushing it. In such cases the proxy reader doesn't work properly. For example with an escape sequence being `ctrl-p,ctrl-q`, when the underlying reader returns `ctrl-p,ctrl-q` at once, the escape sequence isn't detected. This updates the reader to support this use-case and adds unit tests. Signed-off-by: Bilal Amarni <[email protected]>

bamarni · 2020-03-17T20:33:10Z

@dims : I've updated the code as suggested.

@cpuguy83 : I'll try to check this out. One notable difference is that it with your sample it will make multiple read syscalls instead of 1, is that not a concern?

bamarni · 2020-03-17T20:40:54Z

@cpuguy83 : Ah I see, so the bufio.Reader would read 4096 by default and keep it in memory, then we'd iterate one byte at a time.

bamarni · 2020-03-18T20:38:43Z

@cpuguy83 : I've looked at your snippet:

regarding your comment about the test failure because it expects an error, this is due to a difference between bytes.Reader and bufio.Reader, one will return io.EOF when you pass it a p of length 0, while the error wouldn't. See https://play.golang.org/p/YU4NAr0W52o
some other test failures are because the snippet doesn't handle the case where there were a previous call to the escape proxy reader which partially matched an escape sequence, at this point it returned no data. However, on a subsequent call, the proxy reader should not read and return data from the underlying reader straight away, rather, it should fill the p with the partial escape sequence previously matched.

It would require additional logic to fix these 2 (especially the 2nd point), and eventually I'm not sure that the resulting code would be necessarily simpler. Let me know what you think.

bamarni · 2020-03-31T11:29:46Z

Hey @dims @cpuguy83, any news?

dims · 2020-03-31T12:10:03Z

@bamarni waiting on @cpuguy83

AkihiroSuda · 2020-04-07T00:48:00Z

@cpuguy83 PTAL

cpuguy83

LGTM

Should we move this to github.com/moby/term?

dims · 2020-04-16T01:19:11Z

LGTM

+1 to move this to moby/term

thaJeztah

LGTM, thanks!

There used to be a bug in the Moby terminal escape proxy: moby/moby#39728 Since it is now fixed upstream, we can remove the patched version from the codebase. Additionally, this narrows down the Moby dependency to moby/term instead of the broad moby/moby package.

GordonTheTurtle added the status/0-triage label Aug 12, 2019

bamarni mentioned this pull request Aug 13, 2019

taskIO fixes dcos/dcos-core-cli#318

Merged

bamarni force-pushed the term-proxy-pty branch from 7f9f07f to 902392d Compare August 26, 2019 13:10

GordonTheTurtle assigned runcom Aug 27, 2019

thaJeztah mentioned this pull request Aug 27, 2019

pkg/term: refactor TestEscapeProxyRead #39800

Merged

thaJeztah requested changes Aug 27, 2019

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Aug 27, 2019

bamarni force-pushed the term-proxy-pty branch from 902392d to c650123 Compare August 28, 2019 16:07

bamarni force-pushed the term-proxy-pty branch from c650123 to 9b529fe Compare September 7, 2019 21:05

bamarni force-pushed the term-proxy-pty branch from 9b529fe to b327b99 Compare September 8, 2019 14:55

bamarni mentioned this pull request Sep 10, 2019

Windows: skip flaky TestLogBlocking #39887

Merged

bamarni force-pushed the term-proxy-pty branch from b327b99 to b150078 Compare September 17, 2019 08:18

thaJeztah approved these changes Oct 23, 2019

View reviewed changes

dims reviewed Mar 12, 2020

View reviewed changes

pkg/term/proxy_test.go Outdated

Copy link

Contributor

dims Mar 12, 2020

Choose a reason for hiding this comment

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

thanks for the additional tests. helps understand the problem better.

bamarni force-pushed the term-proxy-pty branch from b150078 to 8dd1490 Compare March 17, 2020 20:20

cpuguy83 approved these changes Apr 15, 2020

View reviewed changes

thaJeztah approved these changes Apr 16, 2020

View reviewed changes

thaJeztah merged commit 7d0e939 into moby:master Apr 16, 2020

thaJeztah added this to the 20.03.0 milestone Apr 16, 2020

thaJeztah mentioned this pull request Apr 16, 2020

Support reading multiple bytes in escapeProxy moby/term#4

Merged

bamarni mentioned this pull request Apr 17, 2020

Remove custom escape proxy dcos/dcos-core-cli#431

Merged

Comments

Conversation

bamarni commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah commented Sep 6, 2019

Uh oh!

bamarni commented Sep 7, 2019

Uh oh!

bamarni commented Sep 8, 2019

Uh oh!

bamarni commented Sep 17, 2019

Uh oh!

bamarni commented Sep 26, 2019

Uh oh!

bamarni commented Oct 23, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 23, 2019

Uh oh!

bamarni commented Nov 4, 2019

Uh oh!

bamarni commented Mar 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dims commented Mar 12, 2020

Uh oh!

bamarni commented Mar 17, 2020

Uh oh!

dims commented Mar 17, 2020

Uh oh!

cpuguy83 commented Mar 17, 2020

Uh oh!

bamarni commented Mar 17, 2020

Uh oh!

bamarni commented Mar 17, 2020

Uh oh!

bamarni commented Mar 18, 2020

Uh oh!

bamarni commented Mar 31, 2020

Uh oh!

dims commented Mar 31, 2020

Uh oh!

AkihiroSuda commented Apr 7, 2020

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

dims commented Apr 16, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

bamarni commented Aug 12, 2019 •

edited

Loading