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] Memory Leak in py-tlsh #137

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Conversation

Sryborg
Copy link

@Sryborg Sryborg commented Feb 27, 2024

Providing a fix to the issue #125
Modified the tlshmodule to fix the memory leak.

@jonjoliver
Copy link
Collaborator

Hi Sryborg

Thank you! This is very good!
Could you please make the following changes to the pull request

(1) on lines 384 and 385 you appear to have only changed the identation by 2 char
we prefer to only introduce changes which impact the code
(2) on line 390 you have a small change to the error message.
Do you think this is important enough to change?
(3) on line 397 you have changed the spacing "TLSH_STRING_LEN_REQ-2" => "TLSH_STRING_LEN_REQ - 2"
We really do not want to have changes like that
(4) The Py_XDECREF(asciiStr); look to be the critical changes - though I note that you improved the readibility of the code while doing your bug fix

Please resubmit merge request with changes

@Sryborg
Copy link
Author

Sryborg commented Feb 29, 2024

Hi jonjoliver,
Thankyou for the comments.

(1) Reverted
(2) I had added while debuging, when I was testing with random strings(deliberately shorter TLSH, texts etc). But yeah, in the larger scheme of things, it wouldn't matter, Reverted
(3) Reverted.
(4) 👍🏼 :)

@jonjoliver
Copy link
Collaborator

Hi @Sryborg

Thank you.
It appears that the memory leak is due to PyUnicode_AsASCIIString() leaking memory unless you do Py_XDECREF
As discussed in
https://discuss.python.org/t/memory-leak-with-pyunicode-fromstring-and-pydict-clear/39218

Approved.

Copy link
Collaborator

@jonjoliver jonjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

@jonjoliver jonjoliver merged commit 96536e3 into trendmicro:master Mar 5, 2024
@jonjoliver jonjoliver mentioned this pull request Sep 16, 2024
jonathanoliver2021 pushed a commit to jonathanoliver2021/tlsh that referenced this pull request Sep 18, 2024
16/09/2024
        document what has changed since **4.11.2** on 23/10/2021
        Merge pull request trendmicro#137 - this fixed a memory leak in py-tlsh
        Merge pull request trendmicro#134 - this improved the ifdef WINDOWS to be more portable
@jonathanoliver2021 jonathanoliver2021 mentioned this pull request Sep 18, 2024
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