GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-118814: Fix crash in `_PyArg_UnpackKeywordsWithVararg` by picnixz · Pull Request #122558 · python/cpython

Conversation

@picnixz

@picnixz picnixz commented Aug 1, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The cause of the crash, if I'm not mistaken, is that we currently have:

if (nkwargs) {
	current_arg = ...
}
else {
    current_arg = NULL;
}

if (nargs < vararg && i != vararg) { 
	buf[i] = current_arg;
}
else {
    buf[i + 1] = current_arg;
}

if (current_arg) {
    Py_DECREF(current_arg);
    --nkwargs;
}

If we have the following unrolling:

  • first iteration: current_arg != NULL and use of buf[i + 1] = current_arg
  • second iteration: current_arg == NULL -> buf[i] = NULL so we smash what we just stored, hence the subsequent segfaults. The patch consists of
if (nargs < vararg && i < vararg) {
    if (current_arg != NULL) {
        buf[i] = current_arg;
    }
}

so 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 set buf[i + 1] to a non-NULL current_arg in the previous iteration.

  • Issue: TypeVar with bad arguments segfault/misbehavior #118814

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
picnixz added 5 commits August 1, 2024 14:01
@picnixz
Fix _PyArg_UnpackKeywordsWithVararg().
047d18e 
This fixes an issue where passing positional arguments
as explicit keyword arguments to functions accepting
positional and variadic arguments.
@picnixz
add test
15fc804
@picnixz
add tests
0e601bf
@picnixz
regenerate objects
46e511e
@picnixz
blurb
078e9f7
@picnixz
picnixz requested a review from erlend-aasland as a code owner August 1, 2024 12:20
@bedevere-app bedevere-app Bot mentioned this pull request Aug 1, 2024
TypeVar with bad arguments segfault/misbehavior #118814
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Aug 1, 2024
@picnixz picnixz added the topic-argument-clinic label Aug 1, 2024
sobolevn
sobolevn reviewed Aug 1, 2024
View reviewed changes
Comment thread Include/internal/pycore_global_strings.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
picnixz added 2 commits August 1, 2024 17:38
@picnixz
use better names
f13fa45
@picnixz
update global objects
a4cd890
@picnixz

picnixz commented Aug 1, 2024

Copy link
Copy Markdown
Member Author

Ok, it's not fixed. I found another bug and the fix doesn't work. I need to think a bit more.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz
picnixz marked this pull request as draft August 1, 2024 15:42
@bedevere-app bedevere-app Bot removed the awaiting review label Aug 1, 2024
picnixz added 4 commits August 2, 2024 12:47
@picnixz
fix parsing
c21014e
@picnixz
add tests
c8accdc
@picnixz
remove unused code
659d823
@picnixz
fixup ws
b70263a
@picnixz
picnixz marked this pull request as ready for review August 2, 2024 11:01
@bedevere-app bedevere-app Bot added the awaiting review label Aug 2, 2024
@picnixz

picnixz commented Aug 2, 2024

Copy link
Copy Markdown
Member Author

Hopefully it's now fixed (the issue, if I'm correct, was about the check i != vararg instead of i < vararg (if i comes after vararg, then it should also be offsetted).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Aug 2, 2024

Copy link
Copy Markdown
Contributor

This looks correct; I'll be able to review it in a day or two.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland previously approved these changes Aug 3, 2024
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/getargs.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Aug 3, 2024
picnixz added 3 commits August 4, 2024 10:04
@picnixz
simplify buffer's creation
a59ff66
@picnixz
Use multi-line comments to have collapsible sections
bbd353c
@picnixz
Merge branch 'main' into ac-typevar-crash
b85c199
@serhiy-storchaka
serhiy-storchaka self-requested a review August 4, 2024 08:25
serhiy-storchaka
serhiy-storchaka reviewed Aug 4, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The design of this function is questionable:

  • It seems that vararg is always equal maxpos. 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 buf item 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/getargs.c
// 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);

@serhiy-storchaka serhiy-storchaka Aug 4, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Check the result for NULL.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread Python/getargs.c
}
}
// copy the first arguments until the 'vararg' index
memcpy(buf, args, vararg * sizeof(PyObject *));

@serhiy-storchaka serhiy-storchaka Aug 4, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There may be less than vararg items in args.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@picnixz picnixz Aug 4, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/getargs.c
* buf = [1, 2, NULL, 3, 4].
*/
if (nargs < vararg && i < vararg) {
if (current_arg != NULL) {

@serhiy-storchaka serhiy-storchaka Aug 4, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This leaves buf[i] not initialized, isn't?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@picnixz picnixz Aug 4, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@picnixz

picnixz commented Aug 4, 2024

Copy link
Copy Markdown
Member Author

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
  • Calling ac_tester.vararg_with_many_args(1, 2, 3, 4, e=5) -> says that 'f' is required
  • Calling ac_tester.vararg_with_many_args(1, 2, 3, 4, e=5, f=6) -> segfault

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...)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz

picnixz commented Aug 4, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Placing the tuple of var-positional arguments between keyword-or-positional and keyword-only arguments complicates the code.

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...)))
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz picnixz added the DO-NOT-MERGE label Aug 4, 2024
@picnixz
picnixz marked this pull request as draft August 4, 2024 10:25
@bedevere-app bedevere-app Bot removed the awaiting merge label Aug 4, 2024
@picnixz

picnixz commented Aug 4, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@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...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Aug 4, 2024

Copy link
Copy Markdown
Member

The solution for this issue can be simpler: #122664.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland dismissed their stale review August 4, 2024 22:41

There are unresolved issues

@bedevere-app bedevere-app Bot added the awaiting review label Aug 4, 2024
@picnixz

picnixz commented Aug 7, 2024

Copy link
Copy Markdown
Member Author

Closing in favor of #122664.

If more test coverage is needed, we'll just extract my tests from this one in another PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz picnixz closed this Aug 7, 2024
@picnixz
picnixz deleted the ac-typevar-crash branch August 19, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@sobolevn sobolevn sobolevn left review comments
@erlend-aasland erlend-aasland erlend-aasland left review comments
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner will be requested when the pull request is marked ready for review AA-Turner is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review DO-NOT-MERGE topic-argument-clinic

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@picnixz @erlend-aasland @serhiy-storchaka @sobolevn
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.