Skip to content

bpo-35028: cgi: Fix max_num_fields off by one error#9973

Merged
miss-islington merged 2 commits into
python:masterfrom
matthewbelisle-wf:fix-cgi-max-num-fields-bug
Oct 23, 2018
Merged

bpo-35028: cgi: Fix max_num_fields off by one error#9973
miss-islington merged 2 commits into
python:masterfrom
matthewbelisle-wf:fix-cgi-max-num-fields-bug

Conversation

@matthewbelisle-wf

@matthewbelisle-wf matthewbelisle-wf commented Oct 19, 2018

Copy link
Copy Markdown
Contributor

@matthewbelisle-wf

matthewbelisle-wf commented Oct 19, 2018

Copy link
Copy Markdown
Contributor Author

@ambv @methane Here is the PR I mentioned in #9969 for fixing the off by one error. I also improved the unit tests to make them a little clearer.

@matthewbelisle-wf matthewbelisle-wf changed the title bpo-35028: Fixing cgi.FieldStorage max_num_fields error bpo-35028: Fixing cgi.FieldStorage max_num_fields off by one error Oct 19, 2018
@matthewbelisle-wf

Copy link
Copy Markdown
Contributor Author

I figure this shouldn't need a merge entry since there hasn't been a release since #9660. If it does let me know and I'll add one.

Comment thread Lib/cgi.py Outdated
max_num_fields = self.max_num_fields
sub_max_num_fields = self.max_num_fields
if max_num_fields is not None:
sub_max_num_fields -= len(self.list)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so happy for maintain two local variables.
How about keep only max_num_fields?

            max_num_fields = self.max_num_fields
            if max_num_fields is not None:
                max_num_fields -= len(self.list)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, github removed my one comment.

[snip]
part = klass(..., max_num_fields)
[snip]
if max_num_fields is not None:
    max_num_fields -= 1
    if part.list:
        max_num_fields -= len(part.list)
    if max_num_fields < 0:
        raise ValueError('Max number of fields exceeded')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, that is simpler. I fixed that in a7203c8.

@methane methane added skip news type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Oct 23, 2018
@methane methane changed the title bpo-35028: Fixing cgi.FieldStorage max_num_fields off by one error bpo-35028: Fix max_num_fields off by one error Oct 23, 2018
@methane methane changed the title bpo-35028: Fix max_num_fields off by one error bpo-35028: cgi: Fix max_num_fields off by one error Oct 23, 2018
@miss-islington miss-islington merged commit b79b5c0 into python:master Oct 23, 2018
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @matthewbelisle-wf for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 23, 2018
https://bugs.python.org/issue35028
(cherry picked from commit b79b5c0)

Co-authored-by: matthewbelisle-wf <matthew.belisle@workiva.com>
@bedevere-bot

Copy link
Copy Markdown

GH-10053 is a backport of this pull request to the 3.7 branch.

@bedevere-bot

Copy link
Copy Markdown

GH-10054 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 23, 2018
https://bugs.python.org/issue35028
(cherry picked from commit b79b5c0)

Co-authored-by: matthewbelisle-wf <matthew.belisle@workiva.com>
miss-islington added a commit that referenced this pull request Oct 23, 2018
https://bugs.python.org/issue35028
(cherry picked from commit b79b5c0)

Co-authored-by: matthewbelisle-wf <matthew.belisle@workiva.com>
miss-islington added a commit that referenced this pull request Oct 23, 2018
https://bugs.python.org/issue35028
(cherry picked from commit b79b5c0)

Co-authored-by: matthewbelisle-wf <matthew.belisle@workiva.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants