classification
Title: bool(cgi.FieldStorage(...)) may be False unexpectedly
Type: behavior Stage: patch review
Components: Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: orsenthil Nosy List: gkcn, gvanrossum, orsenthil, python-dev
Priority: normal Keywords: patch

Created on 2013-09-26 23:54 by gvanrossum, last changed 2014-01-12 15:05 by orsenthil. This issue is now closed.

Files
File name Uploaded Description Edit
19097.patch orsenthil, 2013-10-07 02:18 review
19092-v2.diff orsenthil, 2013-10-09 16:54 review
Messages (9)
msg198457 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-09-26 23:54
Check out http://stackoverflow.com/questions/9327597/python-get-does-not-evaluate-to-true-even-though-there-is-an-object 

It turns out a cgi.FieldStorage object may consider itself False even when it has data.  This happens when the initialization decided to use read_single() instead of one of the other ways of reading the field value (read_urlencoded() or read_multi()).  Then self.list remains None, and __nonzero__ returns False no matter what the contents of self.file is.

To make things worse -- or better? -- the Python 3 version still defines __nonzero__() instead of __bool__().
msg198458 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-09-27 00:08
PS. Simple repro:

import cgi; bool(cgi.FieldStorage('logo', u'tux.png'))

This reveals it's been fixed in Python 3 -- it raises TypeError there as it should.  (But __nonzero__ should be deleted, and if someone adds __bool__ they should make sure it raises the same TypeError if self.list is None.)
msg198796 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-10-01 18:06
FieldStorage("foo", "bar") is invalid because the first argument is supposed to
be file-like object and second one headers. Here we are sending invalid headers. By
default, if the headers is none, the content-type is urlencoded.

The right way of using FieldStorage is either using the keyword arguments in a tightly couple manner. Or use it as default instance (fs = cgi.FieldStorage())

So the expectation could be:

>>>fs = cgi.FieldStorage()
>>>bool(fs)
False


>>> # sending correct fs, env
>>> # http://hg.python.org/cpython/file/32de3923bb94/Lib/test/test_cgi.py#l259
>>> fs = cgi.FieldStorage(fs, environ=env)
>>> bool(fs)
True 

The TypeError failure in python3 for bool(fs) is, in the absence of __bool__, it is trying to do
a len() and which ultimately ends up calling keys():
http://hg.python.org/cpython/file/32de3923bb94/Lib/cgi.py#l579

The proper fix in Python3 IMO is the just define __bool__ instead of __nonzero__ and that will be return False instead of TypeError.
msg198802 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-10-01 19:29
I believe you that the example invocation is wrong. But then shouldn't it raise an exception? I still think that if len() raises KeyError, bool() should raise KeyError too.
msg199124 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-10-07 02:18
Hi Guido,

Agree with both your points.

Attaching a patch that fixes this issue.

1. Raises TypeError exception when header is not a Mapping or email.message.Message type.
2. Asserts for fp.read and fp.readline() to assert that fp, the first argument to FieldStorage is a file like object. I would have preferred to assert fp as a type of BytesIO object, but I saw some tests failings, so that could be taken as a separate 3.4 only backwards incompatible improvement (and not bug fix).

Finally, in the cases where read_single() is called for FieldStorage(), bool() raises valid TypeError for empty FieldStorage.

Please review and if it is OK, I can check this in.
msg199311 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-10-09 16:54
Addressing storchaka's review comments. Here is the updated patch.
msg207925 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-12 06:22
New changeset 2d6e7a5659f0 by Senthil Kumaran in branch '2.7':
Adding test coverage for cgi.FieldStorage based on the scenario mentioned in issue #19097
http://hg.python.org/cpython/rev/2d6e7a5659f0
msg207928 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2014-01-12 06:30
This is fixed in all active branches (2.7,3,3 and 3.4). I have addressed all review comments. Thanks.
msg207960 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2014-01-12 15:05
Fixed in:

New changeset a3e49868cfd0 by Senthil Kumaran in branch '3.3':
Issue #19092 - Raise a correct exception when cgi.FieldStorage is given an
http://hg.python.org/cpython/rev/a3e49868cfd0

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

(Thanks for correcting my bad merge Serhiy. I did fix :w and then did a hg resolve --mark, but still ended up with merge lines. I got to look more carefully next time).
History
Date User Action Args
2014-01-12 15:05:04orsenthilsetmessages: + msg207960
2014-01-12 06:30:56orsenthilsetstatus: open -> closed
resolution: fixed
messages: + msg207928
2014-01-12 06:22:31python-devsetnosy: + python-dev
messages: + msg207925
2013-12-14 18:26:19gkcnsetnosy: + gkcn
2013-10-09 16:54:39orsenthilsetversions: + Python 3.3, Python 3.4, - Python 2.6
2013-10-09 16:54:16orsenthilsetfiles: + 19092-v2.diff

messages: + msg199311
2013-10-07 02:18:09orsenthilsetfiles: + 19097.patch
messages: + msg199124

assignee: orsenthil
keywords: + patch
type: behavior
stage: patch review
2013-10-01 19:29:45gvanrossumsetmessages: + msg198802
2013-10-01 18:06:26orsenthilsetnosy: + orsenthil
messages: + msg198796
2013-09-27 01:32:06gvanrossumsetversions: + Python 2.6
2013-09-27 00:08:23gvanrossumsetmessages: + msg198458
versions: - Python 2.6, Python 3.1, Python 3.2, Python 3.3, Python 3.4, Python 3.5
2013-09-26 23:54:42gvanrossumcreate