trendmicro / tlsh

Other
726 stars 135 forks source link

[Fix] Memory Leak in py-tlsh #137

Closed Sryborg closed 4 months ago

Sryborg commented 4 months ago

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

jonjoliver commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.