FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix flaky test test-http-pipeline-flood by dnakamura · Pull Request #2862 · nodejs/node · GitHub

/ node Public

Fix flaky test test-http-pipeline-flood - #2862

Closed
dnakamura wants to merge 1 commit into
nodejs:masterfrom
dnakamura:pipeflood_fix
ConversationCommits (1)ChecksFiles changed
Closed

Fix flaky test test-http-pipeline-flood#2862
dnakamura wants to merge 1 commit into
nodejs:masterfrom
dnakamura:pipeflood_fix

Conversation

dnakamura commented Sep 14, 2015

Copy link
Copy Markdown

rebase of nodejs/node-v0.x-archive#25870
Fixes nodejs/node-v0.x-archive#25732 and nodejs/node-v0.x-archive#25709

So, some background. This test looks to test a feature to prevent a DoS vulnerability. Essentially once native socket write buffers have filled up, the http parser should cork the read stream. (requests in data which has already been read will still be processed)

Current test

  • sets up a http server, with a timeout which destroys the connection after 200ms of inactivity.
  • creates a child which continuously fires requests and does not read responses.
  • On exit asserts that:
    • Inactivity timeout has fired

    • 250 requests have been processed

    • The child process terminated

Issues:

  • Flaky on windows because of very small socket buffers (write buffers fill after only a few responses), depending on timing this means that only ~40 requests will get processed
  • Does not make much sense to assert number of requests greater than a threshold when testing a DoS prevention feature.
  • If feature under test not working, the test case runs until program runs out of memory

New test

  • Set up server without inactivity timer
  • Create child process like before, but with timeout to kill itself after specified amount of time
  • When server sends response, check if native buffer filled (res.write() returns false). On first time:
    • Add listener for data events on the socket which asserts false (backlog logic should have corked the stream so it should never be called)

Fishrock123 added the test Issues and PRs related to the tests. label Sep 14, 2015
mscdex added the http Issues or PRs related to the http subsystem. label Sep 14, 2015
brendanashworth added the windows Issues and PRs related to the Windows platform. label Sep 14, 2015

rvagg commented Sep 15, 2015

Copy link
Copy Markdown
Member

This test is there to confirm that CVE-2013-4450 is addressed, see https://nodejs.org/en/blog/vulnerability/http-server-pipeline-flood-dos/ for some details. Could you confirm that the test correctly identifies the issue (a) exists in older code and (b) is fixed in newer code. Perhaps by trying it against a v0.10.20 build to see it fail and v0.10.21 to see it pass.

ChALkeR mentioned this pull request Sep 20, 2015
Closed

Trott commented Oct 31, 2015

Copy link
Copy Markdown
Member

I had to make a few small changes to the test to be able to try it under v0.10.20 and v0.10.21, but it seems to work as expected. (That is, it fails with 0.10.20 and passes with 0.10.21.)

Changes required to get it to run in those older versions of Node:

  • common.js has template strings and probably other things that don't work in v0.10.x. Fortunately, this test only uses common.PORT so I changed the declaration for common to var common = {PORT: 1337};.
  • .fill() on the Buffer object doesn't return the Buffer in v0.10.x so var bigResponse = new Buffer(10240).fill('x'); had to be split into two lines:
var bigResponse = new Buffer(10240);
bigResponse.fill('x');

Other than that, the test I ran is identical to what @dnakamura submitted in this PR. Here are the results:

$ nvm use 0.10.20
Now using node v0.10.20
$ node test/sequential/test-http-pipeline-flood.js 
ok - child

assert.js:92
  throw new assert.AssertionError({
        ^
AssertionError: false == true
    at process.<anonymous> (/Users/trott/io.js/test/sequential/test-http-pipeline-flood.js:69:5)
    at process.EventEmitter.emit (events.js:95:17)
$ nvm use 0.10.21
Now using node v0.10.21
$ node test/sequential/test-http-pipeline-flood.js 
ok - child
server got 1236 requests
server sent 1157 backlogged requests
ok
$

Note that because I added an extra line, that assertion failure on line 69 is assert(gotTimeout); rather than assert(childClosed);

/cc @rvagg

Trott commented Oct 31, 2015

Copy link
Copy Markdown
Member

@dnakamura Some little style issues in the test file if you run make jslint.

test/sequential/test-http-pipeline-flood.js
   7:0   error  Line 7 exceeds the maximum line length of 80   max-len
   8:0   error  Line 8 exceeds the maximum line length of 80   max-len
   9:0   error  Line 9 exceeds the maximum line length of 80   max-len
   9:95  error  Trailing spaces not allowed                    no-trailing-spaces
  11:0   error  Line 11 exceeds the maximum line length of 80  max-len
  35:4   error  Keyword "if" must be followed by whitespace    space-after-keywords
  35:31  error  Missing space before opening brace             space-before-blocks
  36:6   error  Keyword "if" must be followed by whitespace    space-after-keywords
  36:29  error  Missing space before opening brace             space-before-blocks
  37:96  error  Trailing spaces not allowed                    no-trailing-spaces
  37:0   error  Line 37 exceeds the maximum line length of 80  max-len
  39:0   error  Line 39 exceeds the maximum line length of 80  max-len
  40:0   error  Line 40 exceeds the maximum line length of 80  max-len
  41:40  error  Missing space before opening brace             space-before-blocks
  41:54  error  Missing semicolon                              semi
  41:56  error  Missing semicolon                              semi
  92:25  error  Missing space before opening brace             space-before-blocks
  92:40  error  Missing semicolon                              semi
  92:48  error  Missing semicolon                              semi

Trott added a commit to Trott/io.js that referenced this pull request Oct 31, 2015
test: mark http-pipeline-flood flaky
158c94c 
test-http-pipeline-flood has been flaky on Windows for some time.
Hopefully, nodejs#2862 fixes it and
lands soon, but until then, let's mark it as flaky.
Trott mentioned this pull request Oct 31, 2015
Closed
Trott added a commit that referenced this pull request Nov 2, 2015
test: mark http-pipeline-flood flaky
6d6bc5d 
test-http-pipeline-flood has been flaky on Windows for some time.
Hopefully, #2862 fixes it and
lands soon, but until then, let's mark it as flaky.

PR-URL: #3616
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Trott mentioned this pull request Nov 2, 2015
Closed

Trott commented Nov 2, 2015

Copy link
Copy Markdown
Member

I rebased against current master, fixed the linting issues, and did some additional refactoring. Pull request is #3636.

Trott commented Nov 3, 2015

Copy link
Copy Markdown
Member

It looks like this does not in fact solve the pipeflood flakiness for Windows: https://ci.nodejs.org/job/node-test-binary-windows/189/RUN_SUBSET=1,VS_VERSION=vs2015,label=win2012r2/tapTestReport/test.tap-228/

Trott commented Nov 3, 2015

Copy link
Copy Markdown
Member

Although it's failing differently, I think, so maybe it can be tweaked...

Trott added a commit that referenced this pull request Nov 7, 2015
test: mark http-pipeline-flood flaky
e302c33 
test-http-pipeline-flood has been flaky on Windows for some time.
Hopefully, #2862 fixes it and
lands soon, but until then, let's mark it as flaky.

PR-URL: #3616
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Trott commented Nov 13, 2015

Copy link
Copy Markdown
Member

Closing in favor of #3636

Trott closed this Nov 13, 2015
Trott added a commit to Trott/io.js that referenced this pull request Jan 17, 2016
test: mark http-pipeline-flood flaky
ea58e39 
test-http-pipeline-flood has been flaky on Windows for some time.
Hopefully, nodejs#2862 fixes it and
lands soon, but until then, let's mark it as flaky.

PR-URL: nodejs#3616
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL