Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-97591: In Exception.__setstate__(), acquire strong reference before calling tp_hash slot. #97700

Conversation

ofey404
Copy link
Contributor

@ofey404 ofey404 commented Oct 1, 2022

Added an Py_INCREF / Py_DECREF pair around the call of tp_hash slot in Exception.__setstate__().

If the last reference of dict value is cleared (by dict.clear()), during the __setstate__ method call, it would cause gc crash.

A script to reproduce this is in #97591 :

import gc

class Key(str):
    def __hash__(self):
        d.clear()
        return 0

class Value(str):
    pass

d = {}
d[Key()] = Value()

e = Exception()
e.__setstate__(d)

gc.collect() # Segfault happens here

Similiar issue: #92930

@kumaraditya303 kumaraditya303 added type-bug An unexpected behavior, bug, or error needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 1, 2022
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LG, I just think you could add your name to the NEWS item.

@ofey404
Copy link
Contributor Author

ofey404 commented Oct 2, 2022

@kumaraditya303 I notice the “back port to 3.10/11” tag, and I wonder how to do the back port? If it’s proper I’d like to do it.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM.

Regarding the backport, this is usually taken care of automatically by the Miss-Islington bot. Only if there are git merge problems the bot can't do it, a manual backport is required. In that case you're welcome to try it.

@gvanrossum gvanrossum merged commit d639438 into python:main Oct 2, 2022
@miss-islington
Copy link
Contributor

Thanks @ofey404 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 2, 2022
@bedevere-bot
Copy link

GH-97717 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 2, 2022
…es before calling `tp_hash` slot (pythonGH-97700)

(cherry picked from commit d639438)

Co-authored-by: Ofey Chan <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 2, 2022
…es before calling `tp_hash` slot (pythonGH-97700)

(cherry picked from commit d639438)

Co-authored-by: Ofey Chan <[email protected]>
@gvanrossum
Copy link
Member

You're lucky, the backports happened automatically. They'll be merged as soon as the tests pass (since I pre-approved them).

miss-islington added a commit that referenced this pull request Oct 2, 2022
…ore calling `tp_hash` slot (GH-97700)

(cherry picked from commit d639438)

Co-authored-by: Ofey Chan <[email protected]>
miss-islington added a commit that referenced this pull request Oct 2, 2022
…ore calling `tp_hash` slot (GH-97700)

(cherry picked from commit d639438)

Co-authored-by: Ofey Chan <[email protected]>
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 2, 2022
carljm added a commit to carljm/cpython that referenced this pull request Oct 3, 2022
* main: (2069 commits)
  pythongh-96512: Move int_max_str_digits setting to PyConfig (python#96944)
  pythongh-94808: Coverage: Check picklablability of calliter (python#95923)
  pythongh-94808: Add test coverage for PyObject_HasAttrString (python#96627)
  pythongh-94732: Fix KeyboardInterrupt race in asyncio run_forever() (python#97765)
  Fix typos in `bltinmodule.c`. (pythonGH-97766)
  pythongh-94808: `_PyLineTable_StartsLine` was not used (pythonGH-96609)
  pythongh-97681: Remove Tools/demo/ directory (python#97682)
  Fix typo in unittest docs (python#97742)
  pythongh-97728: Argument Clinic: Fix uninitialized variable in the Py_UNICODE converter (pythonGH-97729)
  pythongh-95913: Fix PEP number in PEP 678 What's New ref label (python#97739)
  pythongh-95913: Copyedit/improve New Modules What's New section (python#97721)
  pythongh-97740: Fix bang in Sphinx C domain ref target syntax (python#97741)
  pythongh-96819: multiprocessing.resource_tracker: check if length of pipe write <= 512 (python#96890)
  pythongh-97706: multiprocessing tests: Delete unused variable `rand` (python#97707)
  pythonGH-85447: Clarify docs about awaiting future multiple times (python#97738)
  [docs] Update logging cookbook with recipe for using a logger like an output… (pythonGH-97730)
  pythongh-97607: Fix content parsing in the impl-detail reST directive (python#97652)
  pythongh-95975: Move except/*/finally ref labels to more precise locations (python#95976)
  pythongh-97591: In `Exception.__setstate__()` acquire strong references before calling `tp_hash` slot (python#97700)
  pythongh-95588: Drop the safety claim from `ast.literal_eval` docs. (python#95919)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants