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-95778: Use a note for the max digits error message by vstinner · Pull Request #96878 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Sep 16, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Add _PyErr_AddNote() function to the internal C API.

  • Issue: CVE-2020-10735: Prevent DoS by large int<->str conversions #95778

Sorry, something went wrong.

Uh oh!

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

All reactions
@vstinner
gh-95778: Use a note for the max digits error message
df000a8
Add _PyErr_AddNote() function to the internal C API.
@vstinner
vstinner requested a review from iritkatriel as a code owner September 16, 2022 18:15
@bedevere-bot bedevere-bot added the awaiting core review label Sep 16, 2022
@vstinner

vstinner commented Sep 16, 2022

Copy link
Copy Markdown
Member Author

@iritkatriel @Zac-HD: This PR adds the _PyErr_AddNote() function to the internal C API. It's a convenient function to add a note to the currently raised exception. The implementation is not trivial: it has to normalize the current exception to get an exception object and then call BaseException_add_note() which is a static function (not available in other C files).

I prefer to call directly BaseException_add_note() rather than calling the add_note() method, in case if a custom exception override the add_note() method. What I really want here is to add a note to the __notes__ attribute (and create it if it doesn't exist).

I'm not sure about the API in case of error. Currently, the function ignores any exception raised while trying to add a note to the exception :-( Do you think that an unraisable exception should be logged here? Or should we replace the currently raised exception with this new one? Maybe it would be acceptable if it's chained with the previous exception?

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Sep 16, 2022

Copy link
Copy Markdown
Member Author

I'm looking at which existing code might benefit from _PyErr_Note(). "Suggestions" added by PyErr_Display() with _Py_Offer_Suggestions() (issue #82711) is not a good fit: this code doesn't modify the exception on purpose. Or do you want to add suggestions in notes? If yes, we should maybe make sure that the note is not added multiple times if PyErr_Display() is called multiple times on the same exception ;-)

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Sep 16, 2022

Copy link
Copy Markdown
Member Author

Maybe _PyPegen_number_token() can use _PyErr_AddNote() for this code?

            // This acts as PyErr_Clear() as we're replacing curexc.
            PyErr_Fetch(&type, &value, &tb);
            Py_XDECREF(tb);
            Py_DECREF(type);
            /* Intentionally omitting columns to avoid a wall of 1000s of '^'s
             * on the error message. Nobody is going to overlook their huge
             * numeric literal once given the line. */
            RAISE_ERROR_KNOWN_LOCATION(
                p, PyExc_SyntaxError,
                t->lineno, -1 /* col_offset */,
                t->end_lineno, -1 /* end_col_offset */,
                "%S - Consider hexadecimal for huge integer literals "
                "to avoid decimal conversion limits.",
                value);
            Py_DECREF(value);
All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel
iritkatriel reviewed Sep 16, 2022
View reviewed changes
Comment thread Include/internal/pycore_pyerrors.h

// Call add_note(note) on the current exception.
// Return -1 on error, or 0 on success.
// Save and restore the current exception.

@iritkatriel iritkatriel Sep 16, 2022

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.

Might be good to mention in the comment that there must be a current exception. I think otherwise it crashes or some assertion fails, not sure.

Sorry, something went wrong.

Uh oh!

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

All reactions
@iritkatriel

iritkatriel commented Sep 16, 2022

Copy link
Copy Markdown
Member

The intention with exception notes was that you use it to add information that is not available at the time the exception is created and raised, but only after you caught it. This doesn't follow that pattern.

Why not just add one or two newlines to the msg?

Zac-HD reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner mentioned this pull request Sep 20, 2022
Use Python 3.11 exception notes (PEP 678) to provide hints on errors #96958
Closed
@vstinner

vstinner commented Sep 20, 2022

Copy link
Copy Markdown
Member Author

The intention with exception notes was that you use it to add information that is not available at the time the exception is created and raised, but only after you caught it. This doesn't follow that pattern. Why not just add one or two newlines to the msg?

Oh, that's far from issue #95778 scope, so I created the new issue #96958 to discuss that instead. I propose to continue the discussion there.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Sep 27, 2022

Copy link
Copy Markdown
Member Author

I close the issue to continue discussing it in #96958.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner closed this Sep 27, 2022
@vstinner
vstinner deleted the maxdigits_note branch September 27, 2022 15:07
@AlexWaygood AlexWaygood mentioned this pull request Mar 14, 2023
gh-102594: PyErr_SetObject adds note to exception raised on normalization error #102675
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel left review comments

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review

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.

3 participants

@vstinner @iritkatriel @bedevere-bot
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 01402503 — 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.