← 返回首页
gh-64490: Fix bugs in argument clinic varargs processing by colorfulappl · Pull Request #32092 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

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

Provide feedback

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

Include my email address so I can be contacted

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
Resetting focus

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

Merged
erlend-aasland merged 28 commits into
python:mainfrom
colorfulappl:unpack_keywords_bugfix
Nov 24, 2022
Merged

gh-64490: Fix bugs in argument clinic varargs processing#32092
erlend-aasland merged 28 commits into
python:mainfrom
colorfulappl:unpack_keywords_bugfix

Conversation

colorfulappl commented Mar 24, 2022
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

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.
Comment thread Tools/clinic/clinic.py Outdated Show resolved Hide resolved

Copy link
Copy Markdown
Contributor

Could you also add a NEWS entry?

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

Copy link
Copy Markdown
Contributor

Thanks; still missing the NEWS entry, though.

Copy link
Copy Markdown
Contributor Author

NEWS added :)

erlend-aasland changed the title bpo-20291: Fix bugs in argument clinic varargs processing gh-64490: Fix bugs in argument clinic varargs processing Aug 11, 2022

Copy link
Copy Markdown
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?

Copy link
Copy Markdown
Contributor

Also, could you please add tests for each case?

Copy link
Copy Markdown
Contributor Author

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 commit 6203962 , 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 commit 6793f38, I'm looking at test_linic.py and will have a try later.

44 hidden items Load more…

Copy link
Copy Markdown
Contributor

Is it still waiting for further changes?

Since gh-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.

Copy link
Copy Markdown
Contributor Author

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

kumaraditya303 self-requested a review November 24, 2022 06:43

Copy link
Copy Markdown
Contributor Author

Another change in e6a4e13:
Argument Clinic does not generate noptargs when there is a vararg and no optional argument.

colorfulappl force-pushed the unpack_keywords_bugfix branch from 6087c2e to c3d6b73 Compare November 24, 2022 11:29

erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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

kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

LGTM

Copy link
Copy Markdown
Contributor

@colorfulappl, please fix the merge conflicts

# Conflicts: # Lib/test/test_clinic.py # Modules/_testclinic.c # Modules/clinic/_testclinic.c.h
erlend-aasland merged commit 0da7283 into python:main Nov 24, 2022

Copy link
Copy Markdown
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.
🐍🍒⛏🤖

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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 request Dec 20, 2022

Copy link
Copy Markdown

GH-100368 is a backport of this pull request to the 3.11 branch.

bedevere-bot removed the needs backport to 3.11 only security fixes label Dec 20, 2022

Copy link
Copy Markdown
Contributor Author

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 pushed a commit that referenced this pull request Dec 28, 2022
serhiy-storchaka removed the needs backport to 3.10 only security fixes label Aug 11, 2024

Copy link
Copy Markdown
Member

It is too later for 3.10.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic-argument-clinic type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

Footer

© 2026 GitHub, Inc.