Skip to content

Fix __cxa_find_matching_catch's memory leak #8947

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 6 commits into from
Jul 9, 2019

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Jul 9, 2019

__cxa_find_matching_catch allocated a buffer of size 4 using malloc only
when it was first called and reused it through the entire program run. And this
malloc'ed memory wasn't freed. Because this is only allocated once, I changed it
to a static allocation using makeStaticAlloc.

Fixes #8919.

@aheejin aheejin requested review from kripken and quantum5 July 9, 2019 00:48
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Nice!

This can be slightly simpler: a new global variable (__cxa_find_matching_catch_buffer) is not needed, as you can write

var buffer = {{{ makeStaticAlloc(4) }}};

inside the one function that uses that buffer (the {{{ .. }}} will be expanded out into the absolute address in memory). lgtm with that.

@kripken
Copy link
Member

kripken commented Jul 9, 2019

Hmm that CI error on test_clear_error_on_massive_static_data is worrying. There should be no change to static memory from this PR, but I think that error means that there is.

I think the issue is that we include that JS file based on DISABLE_EXCEPTION_THROWING and not _CATCHING. I don't remember all the details for the difference (some notes are in settings.js). But I think we do need to ifdef this on DISABLE_EXCEPTION_CATCHING (which is on by default, unlike the other one).

@aheejin
Copy link
Member Author

aheejin commented Jul 9, 2019

Hmm that CI error on test_clear_error_on_massive_static_data is worrying. There should be no change to static memory from this PR, but I think that error means that there is.

I think the issue is that we include that JS file based on DISABLE_EXCEPTION_THROWING and not _CATCHING. I don't remember all the details for the difference (some notes are in settings.js). But I think we do need to ifdef this on DISABLE_EXCEPTION_CATCHING (which is on by default, unlike the other one).

test_clear_error_on_massive_static_data works on my local machine, so it's weird. I addressed feedback and merged incoming just now, so I'll wait for the CI to complete and see if that happens again.

@aheejin aheejin merged commit fe7d36f into emscripten-core:incoming Jul 9, 2019
@aheejin aheejin deleted the fix_exception branch July 9, 2019 23:31
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
`__cxa_find_matching_catch` allocated a buffer of size 4 using `malloc` only
when it was first called and reused it through the entire program run. And this
malloc'ed memory wasn't freed. Because this is only allocated once, I changed it
to a static allocation using `makeStaticAlloc`.

Fixes emscripten-core#8919.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_exceptions_custom fails with lsan
3 participants