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

Fix leaks in gui_helper_3_helper_. #3256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix leaks in gui_helper_3_helper_. #3256

wants to merge 1 commit into from

Conversation

1uc
Copy link
Collaborator

@1uc 1uc commented Nov 27, 2024

The facts are:

  • PyTuple_SetItem steals its third argument 1.
  • hocobj_new returns a new reference.
  • cpp2refstr returns a new reference.

In both cases the reason it leaks is that hocobj_new or cpp2refstr creates a new reference, then the reference count is increased once more. The one reference is stolen by PyTuple_SetItem; but there's still one INCREF which we can't pair up with a DECREF.

The facts are:
  * `PyTuple_SetItem` steals its third argument [1].
  * `hocobj_new` returns a new reference.
  * `cpp2refstr` returns a new reference.

In both cases the reason it leaks is that `hocobj_new` or `cpp2refstr`
creates a new reference, then the reference count is increased once
more. The once reference is stolen by `PyTuple_SetItem`; but there's
still one INCREF which we can't pair up with a matching DECREF.

[1]: https://docs.python.org/3.12/c-api/tuple.html#c.PyTuple_SetItem
Copy link

@1uc 1uc marked this pull request as ready for review November 27, 2024 16:48
Copy link

✔️ e68cc45 -> Azure artifacts URL

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 67.09%. Comparing base (5f0675b) to head (e68cc45).

Files with missing lines Patch % Lines
src/nrnpython/nrnpy_hoc.cpp 0.00% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3256   +/-   ##
=======================================
  Coverage   67.09%   67.09%           
=======================================
  Files         570      570           
  Lines      111082   111081    -1     
=======================================
  Hits        74525    74525           
+ Misses      36557    36556    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@1uc
Copy link
Collaborator Author

1uc commented Nov 28, 2024

@nrnhines The issue is that because we do manual reference counting, the following entirely made up sequence of steps might be possible:

  1. We create a tuple with an element whose reference count is too high by one.
  2. We know that the tuple will be passed to the GUI layer.
  3. We know that the GUI layer steals the element from the tuple, which explains why the element has an inflated reference count.

Or it's just a leak that can be fixed as proposed in this PR. However, since it's GUI related it's likely not covered by the tests, which is why I'd like to ask you if you think merging this PR is a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants