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 by properly closing charsetDetector #6

Merged
merged 1 commit into from Mar 10, 2017

Conversation

mhertsch
Copy link
Contributor

@mhertsch mhertsch commented Mar 9, 2017

Hi,

we detected a memory issue with the latest version of detect-character-encoding and found that the documentation of ucsdet_close specifically mentions:

(see: http://icu-project.org/apiref/icu4c/ucsdet_8h.html#a84dab4d2c56fedb624a01db170ba698c)

Close a charset detector. All storage and any other resources owned by this charset detector will be released. Failure to close a charset detector when finished with it can result in memory leaks in the application.

I tried my hand at fixing the issue. From what I could see, the code change fixed the leak.

Because Nan:ThrowError does not seem to break the execution order of the code in this file, and ucsdet_open returns a null pointer in case of failure, it ought to be enough to close the charsetDetector at the end if it is not a nullptr and therefore was created successfully.

Regards,

Michael

@sonicdoe sonicdoe changed the base branch from master to develop March 10, 2017 11:00
@sonicdoe sonicdoe changed the title Fix: fix memory leak by closing charsetDetector after it was created successfully Fix memory leak by properly closing charsetDetector Mar 10, 2017
@sonicdoe sonicdoe merged commit d443569 into sonicdoe:develop Mar 10, 2017
@sonicdoe
Copy link
Owner

Because Nan:ThrowError does not seem to break the execution order of the code in this file

Thank you for mentioning this. This is a bug on its own, I have fixed it in 2e3aa33.

By the way, because the base branch of this repository is develop (instead of master), I have rebased your branch onto develop. Consequently, your fork’s master branch may look a bit messy now.

You can take a look at the changes in d443569. If it looks good to you, I’ll release it as v0.3.1.

@mhertsch
Copy link
Contributor Author

Yes, looks good to me 👍

@sonicdoe
Copy link
Owner

Released as v0.3.1.

@mhertsch
Copy link
Contributor Author

Thanks for the package update.

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.

None yet

2 participants