[issue12411] cgi.parse_multipart is broken on 3.x

[Irit Katriel]

Change by Irit Katriel :


--
status: open -> pending

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[STINNER Victor]

Change by STINNER Victor :


--
nosy:  -vstinner
status: pending -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Irit Katriel]

Irit Katriel  added the comment:

Senthil, was this last part done in issue29979?

--
nosy: +iritkatriel
status: open -> pending

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Tal Einat]

Changes by Tal Einat talei...@gmail.com:


--
nosy:  -taleinat

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Roundup Robot]

Roundup Robot added the comment:

New changeset a46a0dafcb7a by Senthil Kumaran in branch '3.2':
Issue #12411: Fix to cgi.parse_multipart to correctly use bytes boundaries and
http://hg.python.org/cpython/rev/a46a0dafcb7a

New changeset 59ea872d8b6b by Senthil Kumaran in branch '3.3':
merge from 3.2
http://hg.python.org/cpython/rev/59ea872d8b6b

New changeset 3d7000549eb1 by Senthil Kumaran in branch 'default':
merge from 3.3
http://hg.python.org/cpython/rev/3d7000549eb1

--
nosy: +python-dev

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Senthil Kumaran]

Senthil Kumaran added the comment:

I updated the patch addressing Ezio's comments in the review system and also 
condensed the tests. This fixes the parse_multipart's byte handling at some 
level. 

The docstring of parse_multipart say that, this should be deprecated in favor 
of FieldStorage completely. I will have to trace through the argument and see 
what should be done here and then I shall close this bug.

--
assignee:  - orsenthil
resolution:  - fixed

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Guido van Rossum]

Guido van Rossum added the comment:

Twisted would really like to see this bug fixed.

--
nosy: +gvanrossum

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Glyph Lefkowitz]

Changes by Glyph Lefkowitz gl...@twistedmatrix.com:


--
nosy: +glyph

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Guido van Rossum]

Guido van Rossum added the comment:

Does anyone who was on this bug previously (e.g. the original author
or the reviewers) know what was holding up the patch? Does it need
more review? More tests? Is there any reason to reject fixing this at
all? (I hope not.) As far as replacing the whole thing with a call
into the other code goes, I'm hesitant if only because we don't have
enough unit tests for the edge cases of the implementation that would
be deleted, so if the wholesale replacement were to break user code we
wouldn't find out until after it's been released. Fixing it seems less
risky.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Senthil Kumaran]

Senthil Kumaran added the comment:

I personally think, that the grey area of multipart form encoding and trying 
to use email's updated features for parsing was holding it, not the tests. This 
can be submitted IMO after looking at the related bugs, I shall do a review 
on this one today.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Guido van Rossum]

Guido van Rossum added the comment:

Thank you very much Senthil!

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Florent Xicluna]

Changes by Florent Xicluna florent.xicl...@gmail.com:


--
nosy: +orsenthil

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Tal Einat]

Tal Einat talei...@gmail.com added the comment:

The patch seems broken to me.

In cgi.parse_multipart(), the 'boundary' variable can be a string even though 
it is concatenated to bytes. Its default value is a string, and a string can be 
given via the pdict argument. There is no validity check other than 
valid_boundary(), which allows both string and bytes.

Most of the changes to test_cgi.py are entirely unrelated. The one test added 
which tests cgi.parse_multipart() should fail since it uses a string (not 
bytes) boundary, while the correct boundary for the test is commented out.

I short this patch seems half-baked. IMO reject this patch and fix just the 
bytes/strings issue with cgi.parse_multipart. Or, as mentioned in the comments, 
use FieldStorage to implement it and be done with it.

--
nosy: +taleinat

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Jonas Wagner]

Jonas Wagner ve...@gmx.ch added the comment:

Hi Tal,

Thanks a lot for your feedback.

My primary objective was to increase the test coverage for cgi.py. If it is a 
problem to have the additional tests in this patch I'm happy to create a new 
issue with a separate patch. 

The default value for the boundary was an oversight, sorry for that.

You are right regarding the commented out boundary as well, I forgot to refresh 
the patch. Again, sorry.

Do you think valid_boundary should contain a check to ensure it is a byte 
object?

--
Added file: http://bugs.python.org/file22567/cgi-coverage-2.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Tal Einat]

Tal Einat talei...@gmail.com added the comment:

Yes, please submit the other additional tests in a separate issue.

The default value for boundary should surely be b. A simple test should be 
added where cgi.parse_multipart() uses the default boundary.

If valid_boundary() is used only for cgi.parse_multipart() then it should be 
changed to validate that the boundary is a bytes instance (which would also 
make it simpler). Otherwise (if vaild_boundary() is also used elsewhere) 
cgi.parse_multipart() should itself check that the boundary is indeed a bytes 
instance, throwing a TypeError otherwise.

Tip: You should run the relevant tests, making sure they all pass, before 
submitting a patch. That way you really know that the patch actually works (as 
far as passing all of the tests).

Thanks for adding more stdlib tests :)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Pierre Quentel]

Pierre Quentel pierre.quen...@gmail.com added the comment:

When the FieldStorage class was fixed there was a discussion in issue 4953 
about the module-level functions parse() and parse_multipart(). The code was 
very similar to methods of the FieldStorage class so the idea was to use 
FieldStorage inside the functions

The patch proposed in issue 11066 replaced the code in parse_multipart by just :

def parse_multipart(fp, pdict):
return FieldStorage(fp,environ=pdict)

Did anyone test it ?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Éric Araujo]

Éric Araujo mer...@netwok.org added the comment:

This looks like a conversion bug indeed; network I/O should use bytes.  Strange 
that no-one caught this, but if there was no test and no users, then bugs can 
slip.

See also #11066, #8077, #4953, #6234 (also adding some people from those bugs’ 
nosy fields).

--
nosy: +MHordecki, efosmark, eric.araujo, flox, milesck, quentel
versions: +Python 3.2

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[R. David Murray]

R. David Murray rdmur...@bitdance.com added the comment:

Indeed, Victor's comments on his patch say that he changed code that was in the 
posted patch to say 'line.startswith(b'--')', and the original patch did use 
b'--', but the code he checked in is missing the 'b'.  He also asked for more 
tests.

Victor, any chance you can review this patch, since you were the last one to 
work on the code in question?

--
nosy: +r.david.murray

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[Jonas Wagner]

New submission from Jonas Wagner ve...@gmx.ch:

While writing tests for the cgi module I came across what looks like a 
conversion bug.

cgi.parse_multipart is comparing values it reads from a binary file like with a 
string literal:
line = fp.readline()
...
if line.startswith(--):

This patch adds fixes the issue and adds test for it.

--
components: Library (Lib)
files: cgi-coverage.diff
keywords: patch
messages: 139082
nosy: jonas.wagner
priority: normal
severity: normal
status: open
title: cgi.parse_multipart is broken on 3.x
type: behavior
versions: Python 3.3
Added file: http://bugs.python.org/file22470/cgi-coverage.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12411] cgi.parse_multipart is broken on 3.x

[R. David Murray]

Changes by R. David Murray rdmur...@bitdance.com:


--
nosy: +haypo

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12411
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com