Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#15018 closed (fixed)

LimitedStream.readline deals parameter not correct

Reported by: xjdrew Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords: blocker, regression
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by russellm)

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 aaugustin 5 years ago.
test_and_fix_for_ticket_15018.diff (1.5 KB) - added by aaugustin 5 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 follow-up: Changed 5 years ago by russellm

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to worksforme
  • Status changed from new to 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.

comment:2 in reply to: ↑ 1 Changed 5 years ago by xjdrew

  • Component changed from Uncategorized to Core framework
  • Resolution worksforme deleted
  • Status changed from closed to 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 Changed 5 years ago by russellm

  • Keywords blocker regression added
  • milestone set to 1.3
  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by aaugustin

Changed 5 years ago by aaugustin

comment:4 Changed 5 years ago by aaugustin

  • 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 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

(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 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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