Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-64490: Fix bugs in argument clinic varargs processing#32092

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

colorfulappl
Copy link
Contributor

@colorfulapplcolorfulappl commentedMar 24, 2022•
edited by bedevere-bot
Loading

Fix 3 bugs introduced in#18609.

Bug reason lists in each commit's commit message.

https://bugs.python.org/issue20291

Python allows at most *one* vararg in one function.Remove the improper varargs check which allows function definition like```    *vararg1: object    *vararg2: object```in argument clinic.
Variable `vararg` indicates the index of vararg in parameter list.While copying kwargs to `buf`, the index `i` should not add `vararg`, which leads to an out-of-bound bug.When there are positional args, vararg and keyword args in a function definition, in which case `vararg` > 1, this bug can be triggered.e.g.```    pos: object    *args: object    kw: object```
The calculation of noptargs is incorrect when there is a vararg.This bug prevents parsed arguments passing to XXXX_impl function.e.g.Define function```    *args: object    kw: object```and pass kw=1 to the function, the `kw` parameter won't receive the pass in value.
@erlend-aasland
Copy link
Contributor

Could you also add a NEWS entry?

@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@erlend-aasland
Copy link
Contributor

Thanks; still missing the NEWS entry, though.

@colorfulappl
Copy link
ContributorAuthor

NEWS added :)

erlend-aasland reacted with heart emoji

@erlend-aaslanderlend-aasland changed the titlebpo-20291: Fix bugs in argument clinic varargs processinggh-64490: Fix bugs in argument clinic varargs processingAug 11, 2022
@erlend-aasland
Copy link
Contributor

NEWS added :)

Thanks; that's some entry! :) Could you narrow it down to a couple of concise sentences that describe the actual changes only?

@erlend-aasland
Copy link
Contributor

Also, could you please add tests for each case?

@colorfulappl
Copy link
ContributorAuthor

Also, could you please add tests for each case?

Do you mean put the test cases into test_linic.py?

To see if the OOB bug is fixed in commit6203962 , I should write some code snippet calling function_PyArg_UnpackKeywordsWithVararg, compile it and ensure that it does not crash.

I'm not sure how can I do this without touching the Python vm or library,
though I have wrote a proof-of-concept by adding a built-in function and test it manually:
https://github.com/colorfulappl/cpython/tree/unpack_keywords_oob_bug_poc .

As for commit6793f38, I'm looking at test_linic.py and will have a try later.

erlend-aasland reacted with thumbs up emoji

@erlend-aasland
Copy link
Contributor

Is it still waiting for further changes?

Sincegh-96178 has recently been merged, we finally have a test framework for functional clinic tests, so I'd say this PR should add some tests before we are ready to land.

@colorfulappl
Copy link
ContributorAuthor

I added some test cases and did another change ina48971d, so this PR may be necessary to be reviewed again.@erlend-aasland@isidentical@kumaraditya303

@colorfulappl
Copy link
ContributorAuthor

Another change ine6a4e13:
Argument Clinic does not generatenoptargs when there is a vararg and no optional argument.

Copy link
Contributor

@erlend-aaslanderlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Kumar, Batuhan, do you have further remarks?

Copy link
Contributor

@kumaraditya303kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

@erlend-aasland
Copy link
Contributor

@colorfulappl, please fix the merge conflicts

# Conflicts:#Lib/test/test_clinic.py#Modules/_testclinic.c#Modules/clinic/_testclinic.c.h
@erlend-aaslanderlend-aasland merged commit0da7283 intopython:mainNov 24, 2022
@miss-islington
Copy link
Contributor

Thanks@colorfulappl for the PR, and@erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry,@colorfulappl and@erlend-aasland, I could not cleanly backport this to3.11 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker 0da728387c99fe6c127b070f2d250dc5bdd62ee5 3.11

@miss-islington
Copy link
Contributor

Sorry@colorfulappl and@erlend-aasland, I had trouble checking out the3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport usingcherry_picker on the command line.
cherry_picker 0da728387c99fe6c127b070f2d250dc5bdd62ee5 3.10

@erlend-aasland
Copy link
Contributor

@kumaraditya303 I'm inclined to backport the functional tests for AC to 3.11 and 3.10. What do you think? We've backported tests to increase coverage in the maintained branches before; it would help backporting the AC fixes (and by the way, we should backport the AC bug fixes that we landed earlier today)

colorfulappl added a commit to colorfulappl/cpython that referenced this pull requestDec 20, 2022
@bedevere-bot
Copy link

GH-100368 is a backport of this pull request to the3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11only security fixes labelDec 20, 2022
@colorfulappl
Copy link
ContributorAuthor

I have backported this to 3.11 (#100368).
And since 3.10 Argument Clinic does not support varargs parsing, there is no need to backport this to 3.10, IMHO.

erlend-aasland reacted with thumbs up emoji

erlend-aasland pushed a commit that referenced this pull requestDec 28, 2022
@serhiy-storchakaserhiy-storchaka removed the needs backport to 3.10only security fixes labelAug 11, 2024
@serhiy-storchaka
Copy link
Member

It is too later for 3.10.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@isidenticalisidenticalisidentical approved these changes

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

@kumaraditya303kumaraditya303kumaraditya303 approved these changes

@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka

Assignees

@erlend-aaslanderlend-aasland

Labels
topic-argument-clinictype-bugAn unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

10 participants
@colorfulappl@erlend-aasland@bedevere-bot@kumaraditya303@smontanaro@miss-islington@serhiy-storchaka@isidentical@the-knights-who-say-ni@ezio-melotti
[8]ページ先頭
©2009-2025 Movatter.jp