Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Fix test-http-pipeline-flood.js #25870

Closed
wants to merge 1 commit into from

Conversation

dnakamura
Copy link

Fixes #25732 and #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 stop reading in new data. (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)

@jasnell
Copy link
Member

jasnell commented Aug 21, 2015

@dnakamura ... hey, can I trouble you to open this against the v0.12 branch instead? And, if appropriate, against nodejs/node master if it's an issue there too? We're not going to be landing PRs on master here.

@ChALkeR
Copy link
Member

ChALkeR commented Sep 20, 2015

This is also reported as nodejs/node#2862, no need to keep it open here.

@ChALkeR ChALkeR closed this Sep 20, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants