Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#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 Russell Keith-Magee)

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:

  1. '\n' not in self.buffer
  2. and size is not None
  3. 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)

test_for_ticket_15018.diff (862 bytes ) - added by Aymeric Augustin 13 years ago.
test_and_fix_for_ticket_15018.diff (1.5 KB ) - added by Aymeric Augustin 13 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 by Russell Keith-Magee, 13 years ago

Description: modified (diff)
Resolution: worksforme
Status: newclosed

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.

in reply to:  1 comment:2 by xjdrew, 13 years ago

Component: UncategorizedCore framework
Resolution: worksforme
Status: closedreopened

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 Russell Keith-Magee, 13 years ago

Keywords: blocker regression added
milestone: 1.3
Triage Stage: UnreviewedAccepted

by Aymeric Augustin, 13 years ago

Attachment: test_for_ticket_15018.diff added

by Aymeric Augustin, 13 years ago

comment:4 by Aymeric Augustin, 13 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 Russell Keith-Magee, 13 years ago

Resolution: fixed
Status: reopenedclosed

(In [15222]) Fixed #15018 -- Corrected the handling of LimitedStream? under one edge case involving size restricted buffers and newlines. Thanks to xjdrew for the report, and aaugustin for the patch.

comment:6 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top