LimitedStream.readline deals parameter not correct

[xjdrew]

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

[Russell Keith-Magee]

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.

[xjdrew]

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

[Aymeric Augustin]

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):
   ...

[Russell Keith-Magee]

(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.

[Jacob]

Milestone 1.3 deleted