classification
Title: cgi.FieldStorage, multipart, missing Content-Length
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, berker.peksag, miss-islington, ned.deily, pdgoins-work, quentel, smurfix, srittau, vstinner
Priority: normal Keywords: patch

Created on 2014-02-03 21:33 by srittau, last changed 2019-09-29 13:12 by ned.deily. This issue is now closed.

Files
File name Uploaded Description Edit
cgi-bug.py srittau, 2014-02-03 21:33 test case
cgi.patch smurfix, 2014-06-10 10:19
issue20504.diff berker.peksag, 2015-10-21 17:43 review
Pull Requests
URL Status Linked Edit
PR 10634 closed quentel, 2018-11-21 12:27
PR 10638 merged quentel, 2018-11-21 16:54
PR 15919 merged miss-islington, 2019-09-11 11:06
PR 15920 merged miss-islington, 2019-09-11 11:06
Messages (16)
msg210166 - (view) Author: Sebastian Rittau (srittau) * Date: 2014-02-03 21:33
Consider the attached test case. This test will run fine with Python 2.7, but will fail with Python 3.3. If cgi.FieldStorage() tries to parse a multipart request without a Content-Length header in the main section, segments will have a length of 0.

During the parse process, two instances of FieldStorage are involved. The outer one reads the whole request and creates and delegates reading of the fragment to inner instances.

The main problem is that FieldStorage.read_lines_to_outerboundary() of the inner FieldStorage will read nothing, since self.limit is lower than zero.

    def read_lines_to_outerboundary(self):
        ...
        while 1:
            if _read >= self.limit:
                break
        ...

This happens, since limit is passed when creating the inner instance in FieldStorage.read_multi():

    def read_multi(self, environ, keep_blank_values, strict_parsing):
        ...
            part = klass(self.fp, headers, ib, environ, keep_blank_values,
                         strict_parsing,self.limit-self.bytes_read,
                         self.encoding, self.errors)
        ...

Now, if the total request did not have a Content-Length header, self.limit will be -1.

The naive fix works for the test case, at least, but I don't know if there are other repercussions:

--- /usr/lib/python3.3/cgi.py	2014-02-03 22:31:16.649431125 +0100
+++ cgi.py	2014-02-03 22:32:14.849704379 +0100
@@ -788,7 +788,7 @@
         last_line_lfend = True
         _read = 0
         while 1:
-            if _read >= self.limit:
+            if self.limit >= 0 and _read >= self.limit:
                 break
             line = self.fp.readline(1<<16) # bytes
             self.bytes_read += len(line)
msg220159 - (view) Author: Matthias Urlichs (smurfix) * Date: 2014-06-10 10:17
Actually, the problem is cgi.py around line 550:

        clen = -1
        if 'content-length' in self.headers:
            try:
                clen = int(self.headers['content-length'])
            except ValueError:
                pass
            if maxlen and clen > maxlen:
                raise ValueError('Maximum content length exceeded')
        self.length = clen
        if self.limit is None and clen:
            self.limit = clen

… so self.limit ends up being -1 instead of None. :-/

Somebody please change this test to

        if self.limit is None and clen >= 0:
msg220160 - (view) Author: Matthias Urlichs (smurfix) * Date: 2014-06-10 10:19
Patch attached.
msg220161 - (view) Author: Matthias Urlichs (smurfix) * Date: 2014-06-10 10:19
This also applies to 3.4 and 3.5.
msg220179 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2014-06-10 17:17
Thanks for the report, the test case, and the patch.  It would be helpful if the informal test could be turned into a patch as a formal test in Lib/test_cgi.py and if, Matthias, you would be willing to sign the PSF contributor agreement if you haven't already (https://www.python.org/psf/contrib/contrib-form/).
msg252541 - (view) Author: Sebastian Rittau (srittau) * Date: 2015-10-08 14:10
Is there any progress on this? The fix seems trivial.
msg253303 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-10-21 17:43
The attached patch(cgi.patch) doesn't fix the problem for me: cgi-bug.py still fails with a TypeError. Here is a patch with a test to fix the problem.

With issue20504.diff applied:

$ ./python t.py 
5

(Only changed the "assert len(fields["my-arg"].file.read()) == 5" line with "print(len(fields["my-arg"].file.read()))")
msg253322 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-10-22 07:45
See also issue #24764: "cgi.FieldStorage.read_multi ignores Content-Length" (changeset 11e9f34169d1).
msg253323 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-10-22 07:59
I reviewed issue20504.diff on Rietveld.
msg326895 - (view) Author: Paul Goins (pdgoins-work) Date: 2018-10-02 19:26
I'm just going to ping on this issue.  It looks like this has just slipped off the radar.  I've seen the last diff and the code review, but it seems that this just needs some final follow-up on the code review comments, no?

I could easily do the final cleanup myself, but would prefer to let someone already on this thread take it across the finish line since I didn't do any of the real work for this patch.  If really necessary, I may be able to clean this up if no one else can spare the time.
msg326968 - (view) Author: Matthias Urlichs (smurfix) * Date: 2018-10-03 12:57
Owch, yeah, this fell off the radar.

Anyway, I've signed the CLA, so if somebody could finish and apply this I'd be grateful. Myself, I unfortunately don't have the time.
msg330210 - (view) Author: Pierre Quentel (quentel) * Date: 2018-11-21 17:12
I have submitted PR #10638 to fix this issue.
msg351821 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2019-09-11 11:05
New changeset 2d7cacacc310b65b43e7e2de89e7722291dea6a4 by Benjamin Peterson (Pierre Quentel) in branch 'master':
bpo-20504 : in cgi.py, fix bug when a multipart/form-data request has… (#10638)
https://github.com/python/cpython/commit/2d7cacacc310b65b43e7e2de89e7722291dea6a4
msg351836 - (view) Author: miss-islington (miss-islington) Date: 2019-09-11 12:09
New changeset e3bd941e4e6f4465f17a0e5a4a6bdf4ea0afdd0d by Miss Islington (bot) in branch '3.8':
bpo-20504 : in cgi.py, fix bug when a multipart/form-data request has… (GH-10638)
https://github.com/python/cpython/commit/e3bd941e4e6f4465f17a0e5a4a6bdf4ea0afdd0d
msg351838 - (view) Author: miss-islington (miss-islington) Date: 2019-09-11 12:22
New changeset 99f0e81f43f64b83e18e8cb2a0b66c53a81a74ab by Miss Islington (bot) in branch '3.7':
bpo-20504 : in cgi.py, fix bug when a multipart/form-data request has… (GH-10638)
https://github.com/python/cpython/commit/99f0e81f43f64b83e18e8cb2a0b66c53a81a74ab
msg353492 - (view) Author: Pierre Quentel (quentel) * Date: 2019-09-29 09:31
Now that the PR has been merged, can someone close the issue ?
History
Date User Action Args
2019-09-29 13:12:14ned.deilysetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.9, - Python 3.4, Python 3.5, Python 3.6
2019-09-29 09:31:38quentelsetmessages: + msg353492
2019-09-11 12:22:41miss-islingtonsetmessages: + msg351838
2019-09-11 12:09:26miss-islingtonsetnosy: + miss-islington
messages: + msg351836
2019-09-11 11:06:10miss-islingtonsetpull_requests: + pull_request15561
2019-09-11 11:06:04miss-islingtonsetpull_requests: + pull_request15560
2019-09-11 11:05:56benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg351821
2018-11-21 17:12:56quentelsetmessages: + msg330210
2018-11-21 16:54:58quentelsetpull_requests: + pull_request9885
2018-11-21 12:27:32quentelsetpull_requests: + pull_request9881
2018-11-21 12:00:28quentelsetnosy: + quentel
2018-10-03 12:57:27smurfixsetversions: + Python 3.7, Python 3.8
2018-10-03 12:57:11smurfixsetmessages: + msg326968
2018-10-02 19:26:28pdgoins-worksetnosy: + pdgoins-work
messages: + msg326895
2015-10-22 07:59:37vstinnersetmessages: + msg253323
2015-10-22 07:45:44vstinnersetmessages: + msg253322
2015-10-21 17:43:28berker.peksagsetfiles: + issue20504.diff
versions: + Python 3.6, - Python 3.3
nosy: + berker.peksag

messages: + msg253303

stage: patch review
2015-10-08 14:19:50vstinnersetnosy: + vstinner
2015-10-08 14:10:37srittausetmessages: + msg252541
2014-06-10 17:17:06ned.deilysetnosy: + ned.deily
messages: + msg220179
2014-06-10 10:19:57smurfixsetmessages: + msg220161
versions: + Python 3.4, Python 3.5
2014-06-10 10:19:08smurfixsetfiles: + cgi.patch
keywords: + patch
messages: + msg220160
2014-06-10 10:17:42smurfixsetnosy: + smurfix
messages: + msg220159
2014-02-26 21:54:18srittausettype: behavior
2014-02-03 21:33:19srittaucreate