bool(cgi.FieldStorage(...)) may be False unexpectedly

[gvanrossum]

BPO	19097
Nosy	@gvanrossum, @orsenthil
Files	19097.patch 19092-v2.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/orsenthil'
closed_at = <Date 2014-01-12.06:30:56.211>
created_at = <Date 2013-09-26.23:54:42.380>
labels = ['type-bug']
title = 'bool(cgi.FieldStorage(...)) may be False unexpectedly'
updated_at = <Date 2014-01-12.15:05:04.066>
user = 'https://github.com/gvanrossum'

bugs.python.org fields:

activity = <Date 2014-01-12.15:05:04.066>
actor = 'orsenthil'
assignee = 'orsenthil'
closed = True
closed_date = <Date 2014-01-12.06:30:56.211>
closer = 'orsenthil'
components = []
creation = <Date 2013-09-26.23:54:42.380>
creator = 'gvanrossum'
dependencies = []
files = ['31980', '32021']
hgrepos = []
issue_num = 19097
keywords = ['patch']
message_count = 9.0
messages = ['198457', '198458', '198796', '198802', '199124', '199311', '207925', '207928', '207960']
nosy_count = 4.0
nosy_names = ['gvanrossum', 'orsenthil', 'python-dev', 'gkcn']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue19097'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

[gvanrossum]

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

[gvanrossum]

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

[orsenthil]

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.

[gvanrossum]

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.

[orsenthil]

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.

[orsenthil]

Addressing storchaka's review comments. Here is the updated patch.

[python-dev]

New changeset 2d6e7a5659f0 by Senthil Kumaran in branch '2.7':
Adding test coverage for cgi.FieldStorage based on the scenario mentioned in issue bpo-19097
http://hg.python.org/cpython/rev/2d6e7a5659f0

[orsenthil]

This is fixed in all active branches (2.7,3,3 and 3.4). I have addressed all review comments. Thanks.

[orsenthil]

Fixed in:

New changeset a3e49868cfd0 by Senthil Kumaran in branch '3.3':
Issue bpo-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).