Opened 11 years ago
Closed 5 years ago
#4891 defect closed fixed (fixed)
twisted.web.http does not throttle pipelined requests
Reported by: | ivank | Owned by: | Indradhanush Gupta |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | web | Keywords: | security |
Cc: | jknight, ivank | Branch: | |
Author: |
Description
twisted.web.http loads an unlimited amount of pipelined HTTP requests into memory, even when it is busy processing. This allows for a very easy DoS attack.
My suggestion is to call .pauseProducing()
on the underlying transport when there are 8 pipelined requests queued, and .resumeProducing()
when there are fewer.
Note that even browsers may send as many concurrent pipelined requests as they want to (Opera for example has no limit).
Attachments (3)
Change History (14)
comment:1 Changed 11 years ago by
Cc: | jknight added |
---|
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
Cc: | ivank added |
---|
Changed 9 years ago by
Attachment: | limit_HTTP_requests.patch added |
---|
Added functionality for limiting HTTP requests to 8.
comment:4 Changed 9 years ago by
Keywords: | review added |
---|
comment:5 follow-up: 6 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Indradhanush Gupta |
Thanks for your work on this issue. The attached patch looks like a step in the right direction. All changes and new functionality need to be verified by automated unit tests as well. Can you add unit tests for these changes as well? Thanks again! (Also, a random note, trac separates keywords using spaces, not commas)
comment:6 Changed 9 years ago by
Thanks for the 'random note'. This one is my first patch. Felt nervous. I'll try and do something about the unit tests.
Changed 9 years ago by
Attachment: | limit_HTTP_requests_and_added_tests.patch added |
---|
Made a small change, _pauseFlag = False once the resumeProducing() is about to be called.
comment:7 Changed 9 years ago by
Keywords: | review added |
---|
I've added the test. It creates 9 requests to invoke pauseProducing() and removes 2 of them from the queue to invoke reumeProducing() once again. The problem I am facing is in the first assertTrue() call, even if I'm comparing _pauseFlag with False the test is passing, however _pauseFlag should actually be True once 8 requests are pipelined. I've intentionally left the first assertTrue() statement as :
self.assertTrue(Request._pauseFlag == False)
instead of :
self.assertTrue(Request._pauseFlag == True)
Please someone do have a look at it.
comment:8 Changed 9 years ago by
Owner: | Indradhanush Gupta deleted |
---|
comment:9 Changed 9 years ago by
Owner: | set to Tom Prince |
---|
I am interested in participating in GSoC 13 under Twisted. itamar suggested to assign it to you.
comment:10 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Tom Prince to Indradhanush Gupta |
I think this code is written at the wrong level. The code you have is only in Request
, so that self._quededRequests
is only ever 0
or 1
. The code probably wants to be on HTTPChanel
. I figured this out after actually trying to run the attached code.
Accessing 'http://localhost:8080/never' never completes, so should never complete, and 'http://localhost:8080' returns an empty page. So trying to open 8 copies of the former and then one of the later should hang. But the final one will complete if any of the `/never' requests are cancelled.
A couple of points about the code itself.
- You appear to be using a large (>4) indent in a couple of places.
- You are assigning to a local variable
_pauseFlag
. - Explanation of instance variables should be in the class docstring, rather than comments.
comment:11 Changed 5 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
This is no longer relevant because of #8320, which removed pipelining support from twisted.web by doing exactly this.
It might actually make sense to not handle *any* additional requests while processing one. The main win of the pipelining is usually gonna be the RTT, not processing time. And normally the buffering in TCP will handle storing up a few extra requests. So reading more requests is probably not often a very useful thing to do.
Alternatively, web2's http server implementation is awesome in every way, and in particular, restricts it to 4. :)