Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

matthewbelisle-wf
Copy link
Contributor

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

@matthewbelisle-wf
Copy link
Contributor Author

matthewbelisle-wf commented Oct 19, 2018

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

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
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
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
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
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
@bedevere-bot
Copy link

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

@bedevere-bot
Copy link

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
miss-islington added a commit that referenced this pull request Oct 23, 2018
miss-islington added a commit that referenced this pull request Oct 23, 2018
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