gh-118814: Fix crash in _PyArg_UnpackKeywordsWithVararg
#122558
Conversation
This fixes an issue where passing positional arguments as explicit keyword arguments to functions accepting positional and variadic arguments.
|
Ok, it's not fixed. I found another bug and the fix doesn't work. I need to think a bit more. |
|
Hopefully it's now fixed (the issue, if I'm correct, was about the check |
|
This looks correct; I'll be able to review it in a day or two. |
There was a problem hiding this comment.
This looks good to me, but I'd like @serhiy-storchaka's eyes on this, as he's more knowledgable about the arg parsing corners of the code base.
There was a problem hiding this comment.
The design of this function is questionable:
- It seems that
varargis always equalmaxpos. So this parameter is redundant. - Placing the tuple of var-positional arguments between keyword-or-positional and keyword-only arguments complicates the code.
- This is the only
bufitem that contains non-borrowed reference. This is error prone. - Not always we need a tuple. Creating a tuple can add an overhead. This also makes impossible a fast path.
I will address this in other issue.
| // copy the first arguments until the 'vararg' index | ||
| memcpy(buf, args, vararg * sizeof(PyObject *)); | ||
| // remaining arguments are all considered as variadic ones | ||
| buf[vararg] = _PyTuple_FromArray(&args[vararg], varargssize); |
There was a problem hiding this comment.
Check the result for NULL.
| } | ||
| } | ||
| // copy the first arguments until the 'vararg' index | ||
| memcpy(buf, args, vararg * sizeof(PyObject *)); |
There was a problem hiding this comment.
There may be less than vararg items in args.
There was a problem hiding this comment.
Right and this is something I didn't check. I need to add tests for that (I would like to suggest a way to generate more clinic tests because there is a lack of coverage).
| * buf = [1, 2, NULL, 3, 4]. | ||
| */ | ||
| if (nargs < vararg && i < vararg) { | ||
| if (current_arg != NULL) { |
There was a problem hiding this comment.
This leaves buf[i] not initialized, isn't?
There was a problem hiding this comment.
Urgh... yes. I should probably change the way it's being checked... like a boolean checking whether I replaced something before or not. But depending on whether you want to change the logic of this function, I may delay the commit.
|
Urgh, actually there's another issue: vararg_with_many_args
a: object
b: object
c: object
d: object
*args: object
e: bool
f: bool = False
g: bool = False
I wanted to avoid rewriting the entire function but I think it'll just be easier to just rewrite entirely... I just need to understand the internals a bit more (the variable naming doesn't help IMO...) |
And yes... I'm pretty sure that my issue is about indices... (I like coding and so, but the topic that I always disliked whether it's in maths or in dev is about indices!!! (the irony being I'm doing stuff that requires indices a lot...))) |
|
@serhiy-storchaka I think it's better to re-address the design of the function itself. Do you want to create a separate issue and work on that? (I'm only available today and will be leaving on holidays for roughly 10 days) I should also confess that I don't have the courage to think of it today... |
|
The solution for this issue can be simpler: #122664. |
|
Closing in favor of #122664. If more test coverage is needed, we'll just extract my tests from this one in another PR. |
The cause of the crash, if I'm not mistaken, is that we currently have:
If we have the following unrolling:
buf[i + 1] = current_argbuf[i] = NULLso we smash what we just stored, hence the subsequent segfaults. The patch consists ofso that we are sure that we don't overwrite something. I don't think it's also correct to overwrite
buf[i]if the latter is not NULL but I may be wrong. If I'm wrong, the patch should simply check that we did not setbuf[i + 1]to a non-NULLcurrent_argin the previous iteration.