#15018 closed (fixed)
LimitedStream.readline deals parameter not correct
Reported by: | xjdrew | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Keywords: | blocker, regression | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
I read the code at (http://code.djangoproject.com/browser/django/trunk/django/core/handlers/wsgi.py), LimitedStream.readline function maybe go wrong when the situation meet criteria below:
- '\n' not in self.buffer
- and size is not None
- and len(self.buffer) > len(self.buffer)
it will go to ln102, and leads the parameter of LimitedStream._read_limited is negative, and it will be bypassed to ln82.
I don't read the implementation of self.stream.read, so i'm not sure if it will cause damage.
Please tell me if I misunderstand the code, thanks.
below is the code:
98 def readline(self, size=None): 99 while '\n' not in self.buffer or \ 100 (size is not None and len(self.buffer) < size): 101 if size: 102 chunk = self._read_limited(size - len(self.buffer)) 103 else: 104 chunk = self._read_limited() 105 if not chunk: 106 break 107 self.buffer += chunk 108 sio = StringIO(self.buffer) 109 if size: 110 line = sio.readline(size) 111 else: 112 line = sio.readline() 113 self.buffer = sio.read() 114 return line
77 def _read_limited(self, size=None): 78 if size is None or size > self.remaining: 79 size = self.remaining 80 if size == 0: 81 return '' 82 result = self.stream.read(size) 83 self.remaining -= len(result) 84 return result
best regards,
Drew
Attachments (2)
Change History (8)
follow-up: 2 comment:1 by , 14 years ago
Description: | modified (diff) |
---|---|
Resolution: | → worksforme |
Status: | new → closed |
comment:2 by , 14 years ago
Component: | Uncategorized → Core framework |
---|---|
Resolution: | worksforme |
Status: | closed → reopened |
Replying to russellm:
Your point 3 doesn't make any sense, and I can't work out what it might be a typo for.
If you want to prove there is a problem here, write a formal test case. There are existing tests for LimitedStream covering a wide range of potential use cases; if you can encode your use case as a test that fails, please provide that test and reopen.
Thanks for your comment and the test case suggestion.
I make a mistake about point 3, it should be:
3. and len(self.buffer) > size
I make a usecase as below:
first, I modify the function LimitedStream.readline to add some log as below:
def readline(self, size=None): print "buff length: %s" % len(self.buffer) ####### print buffer size while '\n' not in self.buffer or \ (size is not None and len(self.buffer) < size):
It will print the internal buffer status.
second, I test the class as below:
>>> stream = LimitedStream(StringIO('te\nabcdefghijkhlmnopqrstuvwst'), 10) #### init buffer size => 10 >>> stream.readline(5) buff length: 0 'te\n' >>> stream.readline(1) buff length: 2 'a' >>> stream.readline(1) buff length: 25 ##### buffer overflow 'b'
it's easy to explain. at the beginning, the buffer size is 0, after I call readline(5), the buffer size becomes 2 (equals 5 - len(line) ), then I call readline(1), now it meets the three conditions I list before. so it execute chunk = self._read_limited(size - len(self.buffer)), that is chunk = self._read_limited(-1), for the underlay StringIO, it reads all content. so when I call readline(1) again, you see, the buffer becomes 25, it's more than the size I init.
If I replace StringIO with other stream, the behavior maybe different;
And if the underlay stream is a very very very big file, the program maybe crash.
...
So I suggest, it's a potential bug.
best regards,
Drew
comment:3 by , 14 years ago
Keywords: | blocker regression added |
---|---|
milestone: | → 1.3 |
Triage Stage: | Unreviewed → Accepted |
by , 14 years ago
Attachment: | test_for_ticket_15018.diff added |
---|
by , 14 years ago
Attachment: | test_and_fix_for_ticket_15018.diff added |
---|
comment:4 by , 14 years ago
Has patch: | set |
---|
The boolean expression used to determine if readline should read another chunk looks wrong.
Second attachement contains test case + minimal fix. One of my first contributions to Django, I hope I did not break any guidelines.
I consider the expected behavior is:
readline should stop reading when it has found a newline or it has reached its limit (if applicable)
which is logically equivalent to :
readline should continue reading while is has not found a newline and it has not reached its limit (if applicable)
which is logically equivalent to :
readline should continue reading while is has not found a newline and (it has no limit or it has not reached its limit)
which translates to python code :
while '\n' not in self.buffer and (size is None or len(self.buffer) < size): ...
comment:5 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Your point 3 doesn't make any sense, and I can't work out what it might be a typo for.
If you want to prove there is a problem here, write a formal test case. There are existing tests for LimitedStream covering a wide range of potential use cases; if you can encode your use case as a test that fails, please provide that test and reopen.