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

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

Closed
wants to merge 1 commit into from

Conversation

dnakamura
Copy link
Contributor

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 Fishrock123 added the test Issues and PRs related to the tests. label Sep 14, 2015
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Sep 14, 2015
@brendanashworth brendanashworth added the windows Issues and PRs related to the Windows platform. label Sep 14, 2015
@rvagg
Copy link
Member

rvagg commented Sep 15, 2015

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.

@Trott
Copy link
Member

Trott commented Oct 31, 2015

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
Copy link
Member

Trott commented Oct 31, 2015

@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-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 added a commit that referenced this pull request Nov 2, 2015
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
Copy link
Member

Trott commented Nov 2, 2015

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

@Trott
Copy link
Member

Trott commented Nov 3, 2015

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
Copy link
Member

Trott commented Nov 3, 2015

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-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
Copy link
Member

Trott commented Nov 13, 2015

Closing in favor of #3636

@Trott Trott closed this Nov 13, 2015
Trott added a commit to Trott/io.js that referenced this pull request Jan 17, 2016
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants