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

colorfulappl · 2022-03-24T08:27:37Z

Fix 3 bugs introduced in #18609.

Bug reason lists in each commit's commit message.

https://bugs.python.org/issue20291

Issue: Argument Clinic should understand *args and **kwargs parameters #64490

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.

Tools/clinic/clinic.py

erlend-aasland · 2022-07-20T20:55:06Z

Could you also add a NEWS entry?

# Conflicts: # Lib/test/clinic.test

bedevere-bot · 2022-08-11T09:49:37Z

Most changes to Python require a NEWS entry.

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

erlend-aasland · 2022-08-11T09:54:40Z

Thanks; still missing the NEWS entry, though.

colorfulappl · 2022-08-11T10:01:01Z

NEWS added :)

erlend-aasland · 2022-08-11T13:00:21Z

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 · 2022-08-11T18:42:47Z

Also, could you please add tests for each case?

# Conflicts: # Lib/test/clinic.test

colorfulappl · 2022-08-12T10:30:48Z

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.

erlend-aasland · 2022-11-22T21:54:50Z

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.

colorfulappl · 2022-11-24T05:31:23Z

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

colorfulappl · 2022-11-24T06:47:46Z

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

erlend-aasland

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

kumaraditya303

LGTM

erlend-aasland · 2022-11-24T13:17:45Z

@colorfulappl, please fix the merge conflicts

# Conflicts: # Lib/test/test_clinic.py # Modules/_testclinic.c # Modules/clinic/_testclinic.c.h

miss-islington · 2022-11-24T19:56:53Z

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 · 2022-11-24T19:57:04Z

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

miss-islington · 2022-11-24T19:57:07Z

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

erlend-aasland · 2022-11-24T20:08:33Z

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

…pythonGH-32092) (cherry picked from commit 0da7283)

bedevere-bot · 2022-12-20T12:34:14Z

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

colorfulappl · 2022-12-20T12:46:52Z

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.

…2092) (#100368) (cherry picked from commit 0da7283)

serhiy-storchaka · 2024-08-11T06:46:33Z

It is too later for 3.10.

colorfulappl added 3 commits March 24, 2022 15:21

Fix a bug which allows more than one varargs
6793f38
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.

bedevere-bot added the awaiting review label Mar 24, 2022

the-knights-who-say-ni added the CLA signed label Mar 24, 2022

colorfulappl mentioned this pull request Mar 24, 2022
gh-90370: Avoid temporary varargs tuple creation in argument passing #30312
Closed

isidentical self-requested a review March 24, 2022 13:12

serhiy-storchaka added the topic-argument-clinic label Apr 30, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

erlend-aasland reviewed Jul 20, 2022
View reviewed changes

Tools/clinic/clinic.py Outdated Show resolvedHide resolved

erlend-aasland self-assigned this Jul 20, 2022

erlend-aasland requested a review from serhiy-storchaka July 20, 2022 20:57

colorfulappl added 3 commits August 11, 2022 15:42

Merge branch 'main' into unpack_keywords_bugfix
cefe5e9
# Conflicts: # Lib/test/clinic.test

Regenerate clinic.test
45bde03

Replace pre_buffer to a more descriptive name nargs
bfc5241

📜🤖 Added by blurb_it.
0b1523c

Replace inline literals with double backticks in news
411ad37

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

Rename news
c6a5171

erlend-aasland mentioned this pull request Aug 11, 2022
gh-90928: Statically Initialize the Keywords Tuple in Clinic-Generated Code #95860
Merged

colorfulappl added 2 commits August 12, 2022 17:03

Merge branch 'main' into unpack_keywords_bugfix
42f9273
# Conflicts: # Lib/test/clinic.test

Simplify news
7fdd501

Regenerate clinic.test
0aa4be4

colorfulappl added 3 commits November 23, 2022 12:00

Merge branch 'main' into unpack_keywords_bugfix
0afce9d

Add Argument Clinic functional test cases
87314ff

Fix bug
a48971d

Do not generate unused noptargs
e6a4e13

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

colorfulappl added 3 commits November 24, 2022 14:50

Update news
8592676

Update Lib/test/clinic.test
4c682bf

Delete unnecessary function doc
c3d6b73

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

erlend-aasland approved these changes Nov 24, 2022
View reviewed changes

kumaraditya303 approved these changes Nov 24, 2022
View reviewed changes

colorfulappl added 2 commits November 24, 2022 22:10

Merge branch 'main' into unpack_keywords_bugfix
8b26eb8
# Conflicts: # Lib/test/test_clinic.py # Modules/_testclinic.c # Modules/clinic/_testclinic.c.h

Rerun make clinic
3f88c54

erlend-aasland merged commit 0da7283 into python:mainNov 24, 2022

bedevere-bot removed the awaiting merge label Nov 24, 2022

colorfulappl added a commit to colorfulappl/cpython that referenced this pull request Dec 20, 2022
[3.11]pythongh-64490: Fix bugs in argument clinic varargs processing (…
c420e8f
…pythonGH-32092) (cherry picked from commit 0da7283)

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

erlend-aasland pushed a commit that referenced this pull request Dec 28, 2022
[3.11]gh-64490: Fix bugs in argument clinic varargs processing (GH-3…
a3dbd4c
…2092) (#100368) (cherry picked from commit 0da7283)

serhiy-storchaka removed the needs backport to 3.10 only security fixes label Aug 11, 2024

Uh oh!

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

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

Uh oh!

Conversation

colorfulappl commented Mar 24, 2022• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Jul 20, 2022

Uh oh!

bedevere-bot commented Aug 11, 2022

Uh oh!

erlend-aasland commented Aug 11, 2022

Uh oh!

colorfulappl commented Aug 11, 2022

Uh oh!

erlend-aasland commented Aug 11, 2022

Uh oh!

erlend-aasland commented Aug 11, 2022

Uh oh!

colorfulappl commented Aug 12, 2022

Uh oh!

erlend-aasland commented Nov 22, 2022

Uh oh!

colorfulappl commented Nov 24, 2022

Uh oh!

colorfulappl commented Nov 24, 2022

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

kumaraditya303 left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Nov 24, 2022

Uh oh!

miss-islington commented Nov 24, 2022

Uh oh!

miss-islington commented Nov 24, 2022

Uh oh!

miss-islington commented Nov 24, 2022

Uh oh!

erlend-aasland commented Nov 24, 2022

Uh oh!

bedevere-bot commented Dec 20, 2022

Uh oh!

colorfulappl commented Dec 20, 2022

Uh oh!

serhiy-storchaka commented Aug 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

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