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.

Sign up for GitHub

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

Jump to bottom

PyFunction_GetAnnotations is probably causing ref leaks #97943

Closed
larryhastings opened this issue Oct 5, 2022 · 5 comments
Closed

PyFunction_GetAnnotations is probably causing ref leaks #97943

larryhastings opened this issue Oct 5, 2022 · 5 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@larryhastings
Copy link
Contributor

larryhastings commented Oct 5, 2022 •
edited
Loading

In issue #30409 PyFunction_GetAnnotations was fixed so it wouldn't return a tuple. That's good.

But this change changed the return value. PyFunction_GetAnnotations now returns a new reference to the annotations dict. But according to the documentation, it should return a borrowed reference.

https://docs.python.org/3/c-api/function.html#c.PyFunction_GetAnnotations

If users only read the documentation, and assume that it's correct, they'll leak a reference to the annotation dict.

We could fix this by changing the documentation. However, all the other PyFunction_Get* functions also return borrowed references. So I think the correct fix is to remove the incref.
@larryhastings larryhastings added the type-bug An unexpected behavior, bug, or error label Oct 5, 2022
@larryhastings
Copy link
Contributor Author

IDK how to tag people, so I'll just do it in a comment:

@pablogsal @methane

@larryhastings larryhastings changed the title PyFunction_GetAnnotations should be causing ref leaks PyFunction_GetAnnotations is probably causing ref leaks Oct 5, 2022
@larryhastings
Copy link
Contributor Author

Also, I consider this enough of a bug fix that it should be backported to versions still getting bug fixes.

larryhastings added a commit to larryhastings/cpython that referenced this issue Oct 6, 2022
@larryhastings
pythongh-97943: PyFunction_GetAnnotations should return a borrowed re…
d46f37e 
…ference.

It was returning a new reference, which isn't how it used to work,
and isn't how it's documented.
@bedevere-bot bedevere-bot mentioned this issue Oct 6, 2022
Merged
@ericvsmith
Copy link
Member

I agree that changing back to a borrowed reference to the current version and backporting that behavior it is correct. Many more people would read the documentation and assume it's correct than would read the source code and know that it's a new reference that they need to manage.

@larryhastings
Copy link
Contributor Author

I didn't do a full forensic analysis, but I think what happened here is:

  • func_get_annotations (the getter/setter) returns a fresh reference.
  • func_get_annotations was changed to call this code that says "oh! if it's a tuple, turn it into a dict." this function returns a new reference.
  • PyFunction_GetAnnotations wasn't changed, it was returning the dict.
  • the bug fix was, "oh! just have it call the same function to turn it from a tuple into a dict." but this function returns a new reference, and PyFunction_GetAnnotations shouldn't return a new reference.

pablogsal pushed a commit that referenced this issue Oct 6, 2022
@larryhastings
gh-97943: PyFunction_GetAnnotations should return a borrowed referenc…
6bfb0be 
…e. (#97949)
@bedevere-bot bedevere-bot mentioned this issue Oct 6, 2022
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 6, 2022
@larryhastings @miss-islington
pythongh-97943: PyFunction_GetAnnotations should return a borrowed re…
dd4bbfb 
…ference. (pythonGH-97949)

(cherry picked from commit 6bfb0be)

Co-authored-by: larryhastings <larry@hastings.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 6, 2022
@larryhastings @miss-islington
pythongh-97943: PyFunction_GetAnnotations should return a borrowed re…
e9126fd 
…ference. (pythonGH-97949)

(cherry picked from commit 6bfb0be)

Co-authored-by: larryhastings <larry@hastings.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 6, 2022
@larryhastings @miss-islington
pythongh-97943: PyFunction_GetAnnotations should return a borrowed re…
787cc60 
…ference. (pythonGH-97949)

(cherry picked from commit 6bfb0be)

Co-authored-by: larryhastings <larry@hastings.org>
@bedevere-bot bedevere-bot mentioned this issue Oct 6, 2022
Merged
miss-islington added a commit that referenced this issue Oct 6, 2022
@miss-islington @larryhastings
gh-97943: PyFunction_GetAnnotations should return a borrowed referenc…
33cf0a6 
…e. (GH-97949)

(cherry picked from commit 6bfb0be)

Co-authored-by: larryhastings <larry@hastings.org>
carljm added a commit to carljm/cpython that referenced this issue Oct 6, 2022
@carljm
Merge branch 'main' into dictwatch
e5d2169 
* main:
  fixes pythongh-96078: os.sched_yield release the GIL while calling sched_yield(2). (pythongh-97965)
  pythongh-65961: Do not rely solely on `__cached__` (pythonGH-97990)
  pythongh-97850: Remove the open issues section from the import reference (python#97935)
  Docs: pin sphinx-lint (pythonGH-97992)
  pythongh-94590: add signatures to operator itemgetter, attrgetter, methodcaller (python#94591)
  Add Pynche's move to the What's new in 3.11 (python#97974)
  pythongh-97781: Apply changes from importlib_metadata 5. (pythonGH-97785)
  pythongh-86482: Document assignment expression need for ()s (python#23291)
  pythongh-97943: PyFunction_GetAnnotations should return a borrowed reference. (python#97949)
  pythongh-94808: Coverage: Test that maximum indentation level is handled (python#95926)
ambv pushed a commit that referenced this issue Oct 7, 2022
@miss-islington @larryhastings
[3.10] gh-97943: PyFunction_GetAnnotations should return a borrowed r…
e0e303a 
…eference. (GH-97949) (GH-97989)

gh-97943: PyFunction_GetAnnotations should return a borrowed reference. (GH-97949)
(cherry picked from commit 6bfb0be)

Co-authored-by: larryhastings <larry@hastings.org>
carljm added a commit to carljm/cpython that referenced this issue Oct 8, 2022
@carljm
Merge branch 'main' into typewatch
415ed49 
* main:
  pythonGH-97002: Prevent `_PyInterpreterFrame`s from backing more than one `PyFrameObject` (pythonGH-97996)
  pythongh-97973: Return all necessary information from the tokenizer (pythonGH-97984)
  fixes pythongh-96078: os.sched_yield release the GIL while calling sched_yield(2). (pythongh-97965)
  pythongh-65961: Do not rely solely on `__cached__` (pythonGH-97990)
  pythongh-97850: Remove the open issues section from the import reference (python#97935)
  Docs: pin sphinx-lint (pythonGH-97992)
  pythongh-94590: add signatures to operator itemgetter, attrgetter, methodcaller (python#94591)
  Add Pynche's move to the What's new in 3.11 (python#97974)
  pythongh-97781: Apply changes from importlib_metadata 5. (pythonGH-97785)
  pythongh-86482: Document assignment expression need for ()s (python#23291)
  pythongh-97943: PyFunction_GetAnnotations should return a borrowed reference. (python#97949)
mpage pushed a commit to mpage/cpython that referenced this issue Oct 11, 2022
@larryhastings @mpage
pythongh-97943: PyFunction_GetAnnotations should return a borrowed re…
149ed7f 
…ference. (python#97949)
@hauntsaninja
Copy link
Contributor

Thanks for spotting, looks like it's been fixed and backported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ericvsmith @hauntsaninja @larryhastings