Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Attach HTTP stream parsing fails if TTY is enabled #97

Open
ximon18 opened this issue Jun 10, 2020 · 0 comments
Open

BUG: Attach HTTP stream parsing fails if TTY is enabled #97

ximon18 opened this issue Jun 10, 2020 · 0 comments

Comments

@ximon18
Copy link

ximon18 commented Jun 10, 2020

From the Docker API documentation for attaching to a container:

Stream format
When the TTY setting is disabled in POST /containers/create, the stream over the hijacked connected is multiplexed to separate out stdout and stderr. The stream consists of a series of frames, each containing a header and a payload.

This is supported by dockworker (0.0.18).

And:

Stream format when using a TTY
When the TTY setting is enabled in POST /containers/create, the stream is not multiplexed. The data exchanged over the hijacked connection is simply the raw data from the process PTY and client's stdin.

This second mode is NOT supported by dockworker (0.0.18). What happens is that AttachResponseIter::next() fails because it assumes that the response consists of frames starting with an 8 byte header, but when using ContainerCreateOptions::tty(true) those 8 bytes are actual output from the container, not a header. In my case it interpreted the frame size as a very large number and failed to allocate enough memory for the frame.

I'm not sure how to properly solve this as the stream format doesn't contain a magic value that can be used to detect framed mode, and the knowledge that the container was created with TTY mode is far away from the container attach code which only knows about a container ID. The code leans heavily on implementing standard Rust traits and so the interfaces cannot be changed to include a flag to tell the parsing how to behave. With some refactoring, perhaps this was intended in the improve/attach-api branch?, something can be done I'm sure, but being new to Rust I'm not ready to offer an implementation yet myself.

I have created a proof-of-concept to show tests the first four bytes of the stream header which should always be 0x0000, 0x1000 or 0x2000, and while these are non-printable ASCII codes, I think they represent real UTF-8 code points and so could appear in a multiplexed non-framed UTF-8 stream, i.e. this hack won't work in that case. I didn't want to submit it as a PR, as it doesn't feel like the right solution, but it does show that the problem can be fixed.

See: https://github.com/eldesh/dockworker/compare/master...ximon18:patch-1?expand=1

I tested this patch on a local build of v0.0.18 successfully both with TTY false and TTY true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant