fixes: #38745 lost UnixConn features in listener socket read hack by lifubang · Pull Request #38928 · moby/moby

lifubang · 2019-03-23T01:06:51Z

Signed-off-by: Lifubang lifubang@acmcoder.com

- What I did
As mentioned in #38745 , docker will exit with an error when stdin is not fully consumed.
It's very import for my OJ (Online Judge) product, so I want to fix it.

My test case running command is always like this:

cat 1.in | docker run -i --name=test101 acmcoder/ab:once

If the code in acmcoder/ab:once can't consume all the data in stdin, it will get an error.

- How I did it
I have tested it with steps as follows:

with -H :2375, there is no error:

root@test:~# head -c 262145 /dev/zero | docker -H :2375 run --rm -i busybox ls /var
spool
www

with -H unix:///var/run/docker.sock

root@test:~# head -c 262145 /dev/zero | docker -H unix:///var/run/docker.sock run --rm -i busybox ls /var
spool
www
read unix @->/var/run/docker.sock: read: connection reset by peer

The only difference between tcp and unix is hack.
In hack, we just use net.Conn interface, it will cause net.UnixConn's features lost.
So, use net.UnixConn instead of net.Conn.

- How to verify it

head -c 262145 /dev/zero | docker -H unix:///var/run/docker.sock run --rm -i busybox ls /var
spool
www

There is no error.

** Further more **
I don't know what features are lost without net.UnixConn. If some one can tell me, I'll appreciate very much.

- Description for the changelog
Use net.UnixConn instead of net.Conn in dockerd listener's data reading hack.

- A picture of a cute animal

lifubang · 2019-03-23T01:13:37Z

For 262144 bytes = 256KB
I think the close action is in here:
https://github.com/golang/go/blob/master/src/net/http/server.go#L1067-L1077

// maxPostHandlerReadBytes is the max number of Request.Body bytes not
// consumed by a handler that the server will read from the client
// in order to keep a connection alive. If there are more bytes than
// this then the server to be paranoid instead sends a "Connection:
// close" response.
//
// This number is approximately what a typical machine's TCP buffer
// size is anyway.  (if we have the bytes on the machine, we might as
// well read them)
const maxPostHandlerReadBytes = 256 << 10

codecov · 2019-03-23T15:19:28Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d7ab8ad). Click here to learn what that means.
The diff coverage is 59.57%.

@@            Coverage Diff            @@
##             master   #38928   +/-   ##
=========================================
  Coverage          ?   36.91%           
=========================================
  Files             ?      614           
  Lines             ?    45406           
  Branches          ?        0           
=========================================
  Hits              ?    16762           
  Misses            ?    26357           
  Partials          ?     2287

lifubang · 2019-03-25T01:29:24Z

@runcom @cpuguy83 It's from your commit in 2016.
3d6f598#diff-dc113ba503f0475715914adbd2debc60
Could you please take a look, it puzzle me very much.
For this case, why using net.UnixConn can work, but net.Conn can't work?

cpuguy83 · 2019-03-28T22:42:12Z

cmd/dockerd/hack/malformed_host_override.go

Nit, this is never nil (else it would panic on the type assertion).

Thank you for your review. I remove the c == nil check and add a ok check.

cpuguy83 · 2019-03-28T22:45:36Z

cmd/dockerd/hack/malformed_host_override.go

Seems weird to allocate buffers and only use them to get the length.

buf is used in here copy(b, buf), but I also change to a more reasonable way.

Ah your right, thinking of append, but copy is ok.
Can we update the function name? To me a check shouldn't be modifying things and makes it confusing... maybe stripInvalidHeader

maybe stripInvalidHeader

OK

cpuguy83 · 2019-03-29T19:34:04Z

cmd/dockerd/hack/malformed_host_override_test.go

Not sure I'm following why this is added.

Maybe time is too long to remember the reason.

cpuguy83 · 2019-03-29T19:36:21Z

docker-py really does not seem to like this change for some reason.

I'm wondering if we should consider just removing this hack alltogether at this point. As I recall this was an issue with clients built with < go1.8 which is way out of support...
The flip side of this is it could trigger a breaking change for people that are just using a client that hasn't been re-built in some time.

Signed-off-by: Lifubang <lifubang@acmcoder.com>

lifubang · 2019-03-30T01:25:04Z

docker-py really does not seem to like this change for some reason.

I'll try retest for one more time.

I'm wondering if we should consider just removing this hack alltogether at this point. As I recall this was an issue with clients built with < go1.8 which is way out of support...
The flip side of this is it could trigger a breaking change for people that are just using a client that hasn't been re-built in some time.

If this hack removed, it works fine. I quite agree with you to remove this hack. Should need any other maintainers to view this remove?

thaJeztah · 2019-04-11T18:49:52Z

Discussing in the maintainers meeting with @cpuguy83 @wk8, @kolyshkin , and we think it should be ok to remove the hack altogether; this hack was there for clients compiled with Go 1.5 or older, which is 4 years old; if you're using tools that haven't been updated in 4 years, I think that's not something we can take into account.

We should add something in the release notes though, just in case someone somewhere still has such old tools 🤗

thaJeztah · 2019-04-15T08:26:52Z

I'm taking care of removing the hack in #39076, so let me close this one.

Thank you so much @lifubang for working on this; it's appreciated!!

GordonTheTurtle added the status/0-triage label Mar 23, 2019

lifubang force-pushed the unixsocket branch 2 times, most recently from afafc8b to 8f2d497 Compare March 23, 2019 15:19

cpuguy83 added rebuild/janky labels Mar 28, 2019

GordonTheTurtle removed the rebuild/janky label Mar 28, 2019

cpuguy83 added status/2-code-review rebuild/janky and removed status/0-triage labels Mar 28, 2019

GordonTheTurtle removed the rebuild/janky label Mar 28, 2019

cpuguy83 reviewed Mar 28, 2019

View reviewed changes

cpuguy83 requested changes Mar 28, 2019

View reviewed changes

lifubang force-pushed the unixsocket branch 2 times, most recently from 79605e5 to d449b0e Compare March 29, 2019 00:23

cpuguy83 reviewed Mar 29, 2019

View reviewed changes

lifubang force-pushed the unixsocket branch from d449b0e to ca9aae2 Compare March 30, 2019 01:20

fix lost UnixConn features in socket read hack

ca9aae2

Signed-off-by: Lifubang <lifubang@acmcoder.com>

GordonTheTurtle assigned MHBauer Apr 7, 2019

thaJeztah added rebuild/* and removed rebuild/windowsRS5-process labels Apr 10, 2019

GordonTheTurtle removed the rebuild/* label Apr 10, 2019

This was referenced Apr 15, 2019

Remove hack MalformedHostHeaderOverride #39076

Merged

Refactor Read function in malformed_host_override.go #39046

Closed

thaJeztah closed this Apr 15, 2019

Conversation

lifubang commented Mar 23, 2019

Uh oh!

lifubang commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lifubang commented Mar 25, 2019

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!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Mar 29, 2019

Uh oh!

lifubang commented Mar 30, 2019

Uh oh!

thaJeztah commented Apr 11, 2019

Uh oh!

thaJeztah commented Apr 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lifubang commented Mar 23, 2019 •

edited

Loading

codecov bot commented Mar 23, 2019 •

edited

Loading