gh-97591: In `Exception.__setstate__()`, acquire strong reference before calling `tp_hash` slot. by ofey404 · Pull Request #97700 · python/cpython · GitHub
Skip to content

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

Merged
gvanrossum merged 4 commits into
python:mainfrom
ofey404:ofey404/gh-97591-exception-setstate-strong-reference
Oct 2, 2022
Merged

gh-97591: In Exception.__setstate__(), acquire strong reference before calling tp_hash slot.#97700
gvanrossum merged 4 commits into
python:mainfrom
ofey404:ofey404/gh-97591-exception-setstate-strong-reference

Conversation

@ofey404

@ofey404 ofey404 commented Oct 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@ofey404
ofey404 requested a review from iritkatriel as a code owner October 1, 2022 08:53
@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
gvanrossum
gvanrossum approved these changes Oct 1, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

Comment thread Misc/NEWS.d/next/Core and Builtins/2022-10-01-08-55-09.gh-issue-97591.pw6kkH.rst Outdated
@ofey404 @gvanrossum
Update Misc/NEWS.d/next/Core and Builtins/2022-10-01-08-55-09.gh-issu…
b5dfd41 
…e-97591.pw6kkH.rst

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
@ofey404

ofey404 commented Oct 2, 2022

Copy link
Copy Markdown
Contributor Author

gvanrossum
gvanrossum approved these changes Oct 2, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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

miss-islington commented Oct 2, 2022

Copy link
Copy Markdown
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

bedevere-bot commented Oct 2, 2022

Copy link
Copy Markdown

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

bedevere-bot commented Oct 2, 2022

Copy link
Copy Markdown

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
@ofey404 @miss-islington
pythongh-97591: In Exception.__setstate__() acquire strong referenc…
06b67fb 
…es before calling `tp_hash` slot (pythonGH-97700)

(cherry picked from commit d639438)

Co-authored-by: Ofey Chan <ofey206@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 2, 2022
@ofey404 @miss-islington
pythongh-97591: In Exception.__setstate__() acquire strong referenc…
c35e9d7 
…es before calling `tp_hash` slot (pythonGH-97700)

(cherry picked from commit d639438)

Co-authored-by: Ofey Chan <ofey206@gmail.com>
@gvanrossum

gvanrossum commented Oct 2, 2022

Copy link
Copy Markdown
Member

miss-islington added a commit that referenced this pull request Oct 2, 2022
@miss-islington @ofey404
gh-97591: In Exception.__setstate__() acquire strong references bef…
c6fcbb4 
…ore calling `tp_hash` slot (GH-97700)

(cherry picked from commit d639438)

Co-authored-by: Ofey Chan <ofey206@gmail.com>
miss-islington added a commit that referenced this pull request Oct 2, 2022
@miss-islington @ofey404
gh-97591: In Exception.__setstate__() acquire strong references bef…
dbde686 
…ore calling `tp_hash` slot (GH-97700)

(cherry picked from commit d639438)

Co-authored-by: Ofey Chan <ofey206@gmail.com>
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 2, 2022
@ofey404 @serhiy-storchaka
pythongh-97591: In Exception.__setstate__() acquire strong referenc…
e401b65 
…es before calling `tp_hash` slot (python#97700)
carljm added a commit to carljm/cpython that referenced this pull request Oct 3, 2022
@carljm
Merge branch 'main' into dictwatch
e83abac 
* 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

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ofey404 @miss-islington @bedevere-bot @gvanrossum @kumaraditya303