Skip to content

fix flaky test TestImportFileWithMessage#23046

Merged
LK4D4 merged 1 commit intomoby:masterfrom
mountkin:fix-23045
May 31, 2016
Merged

fix flaky test TestImportFileWithMessage#23046
LK4D4 merged 1 commit intomoby:masterfrom
mountkin:fix-23045

Conversation

@mountkin
Copy link
Copy Markdown
Contributor

@mountkin mountkin commented May 27, 2016

- What I did
Fixes #23045
- How I did it
Fixed the index out of range error in the MalformedHostHeaderOverrideConn reader.

- How to verify it
TESTFLAGS='-check.f DockerSuite.TestImportFileWithMessage' make test-integration-cli
- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Shijiang Wei mountkin@gmail.com

@AkihiroSuda
Copy link
Copy Markdown
Member

(non-binding) could you please add a test to malformed_host_override_test.go?

@mountkin
Copy link
Copy Markdown
Contributor Author

Test updated.
It seems that the former test of TestHeaderOverrideHack is incorrect. It only tested the first case, all the other cases were constantly passing because length of the read slice was truncated to zero after the first test.
Moreover, the former test reused the pipe for each test, the 5th test would read the remaining data in the pipe that had been written by the 4th case rather than the real test data of itself.

@mountkin
Copy link
Copy Markdown
Contributor Author

Janky failure seems unrelated to this change.

@vdemeester
Copy link
Copy Markdown
Member

/cc @runcom

@runcom
Copy link
Copy Markdown
Member

runcom commented May 27, 2016

How do I trigger that index out of range error? Ping @cpuguy83 @LK4D4 also

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@runcom This triggered the index out of range

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I meant, this index out of range was from some failed test in some pull request or?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

More precisely, if '\n' appears in the last 7 bytes of the first 4096 bytes, the index out of range error will be triggered because i+n will be greater than 4095.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: Shijiang Wei <mountkin@gmail.com>
@runcom
Copy link
Copy Markdown
Member

runcom commented May 31, 2016

alright, I got the panic as well, I'll test this out now

@runcom
Copy link
Copy Markdown
Member

runcom commented May 31, 2016

LGTM thanks!!! ping @cpuguy83 @LK4D4

@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented May 31, 2016

LGTM

@LK4D4 LK4D4 merged commit 0b5e84c into moby:master May 31, 2016
@mountkin mountkin deleted the fix-23045 branch June 1, 2016 15:16
runcom added a commit to runcom/docker that referenced this pull request Jun 8, 2016
…ents

Upstream reference: moby#22000
Upstream reference: moby#23046
Upstream reference: moby#22888

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@bboreham
Copy link
Copy Markdown
Contributor

Note the title understates what this fixes - a suitably malformed request would have crashed the daemon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: TestImportFileWithMessage

7 participants