gh-64490: Fix bugs in argument clinic varargs processing #32092
Conversation
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.
|
Could you also add a NEWS entry? |
# Conflicts: # Lib/test/clinic.test
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
|
Thanks; still missing the NEWS entry, though. |
|
NEWS added :) |
erlend-aasland
changed the title
Thanks; that's some entry! :) Could you narrow it down to a couple of concise sentences that describe the actual changes only? |
|
Also, could you please add tests for each case? |
# Conflicts: # Lib/test/clinic.test
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 I'm not sure how can I do this without touching the Python vm or library, As for commit 6793f38, I'm looking at test_linic.py and will have a try later. |
Thanks for the ping @erlend-aasland, and thanks for the PR @colorfulappl. I've gone through the changes, and they all look good to me. Quick question below:
|
(I'm working my way through some PRs which have been approved and are labeled "awaiting merge", hence my seemingly bolt from the blue comment. Why? Read here.) I looked through the comments. 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. |
|
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 |
|
Another change in e6a4e13: |
colorfulappl
force-pushed
the
unpack_keywords_bugfix
branch
from
6087c2e
to
c3d6b73
Compare
|
@colorfulappl, please fix the merge conflicts |
# Conflicts: # Lib/test/test_clinic.py # Modules/_testclinic.c # Modules/clinic/_testclinic.c.h
|
Thanks @colorfulappl for the PR, and @erlend-aasland for merging it |
|
Sorry, @colorfulappl and @erlend-aasland, I could not cleanly backport this to |
|
Sorry @colorfulappl and @erlend-aasland, I had trouble checking out the |
|
@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) |
Fix 3 bugs introduced in #18609.
Bug reason lists in each commit's commit message.
https://bugs.python.org/issue20291