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

BUG: Memory leak bugs due to returned new reference is not decreased on failure (static analyzer reports) #35

Closed
Snape3058 opened this issue May 24, 2022 · 5 comments · Fixed by #37

Comments

@Snape3058
Copy link

Returning a new reference here:

pyxattr/xattr.c

Line 632 in c3466e7

my_tuple = Py_BuildValue("yy#", name, buf_val, nval);

Variable my_tuple goes out of scope without decreasing the refcnt.

pyxattr/xattr.c

Line 639 in c3466e7

goto free_buf_val;

Internal Report ID: 19f80b


Returning a new reference here:

pyxattr/xattr.c

Line 1185 in c3466e7

PyObject *m = PyModule_Create(&xattrmodule);

Error handling code after goto target does not decrease the refcnt. (also other gotos in this function)

pyxattr/xattr.c

Line 1201 in c3466e7

goto err_out;

Internal Report ID: 1eed62

@iustin
Copy link
Owner

iustin commented May 25, 2022

Thanks for the report, much appreciated! I'll take a look in some while, right now away from proper machine to test.

Just for my curiosity, which analyser did you use? I'd like to integrate it in GitHub hooks or at least release process, if it's freely available.

@Snape3058
Copy link
Author

It is an experimental analyzer developed on the top of Clang Static Analyzer. When the tool is publicly available, I will reply to this issue to tell you. Currently, I am busy writing the paper for this research.

@iustin
Copy link
Owner

iustin commented May 25, 2022

Sounds good, thank you - and thanks for finding the bugs!

iustin added a commit that referenced this issue Oct 9, 2022
Issue #35 found two instances of memory leaks. This fixes the first one, which arguably is a more realistic case.
@iustin
Copy link
Owner

iustin commented Oct 10, 2022

I've committed a fix for the first issue, which was very clear. For the second batch of issues, in the module initalisation, I'm not entirely sure what's the correct way of handling initialisation error - is deconstructing (via ref count decrease) the module the right thing? It seems so, but testing behaviour here is hard.

@iustin
Copy link
Owner

iustin commented Oct 10, 2022

Ah, actually the tutorial shows exactly this. Then it's a clear and simple fix.

iustin added a commit that referenced this issue Oct 10, 2022
Together with the commit in #36, this should fix issue #35,
@iustin iustin linked a pull request Oct 10, 2022 that will close this issue
iustin added a commit that referenced this issue Oct 10, 2022
Together with the commit in #36, this should fix #35.
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 a pull request may close this issue.

2 participants