Fix flaky test test-http-pipeline-flood

[dnakamura]

rebase of nodejs/node-v0.x-archive#25870
Fixesnodejs/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)

[rvagg]

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]

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]

@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]

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

[Trott]

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]

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

[Trott]

Closing in favor of #3636