python / cpython

The Python programming language
https://www.python.org
Other
62.33k stars 29.94k forks source link

PyFunction_GetAnnotations is probably causing ref leaks #97943

Closed larryhastings closed 1 year ago

larryhastings commented 1 year ago

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 commented 1 year ago

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

@pablogsal @methane

larryhastings commented 1 year ago

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

ericvsmith commented 1 year ago

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 commented 1 year ago

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

hauntsaninja commented 1 year ago

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