loader: free handle and type maps of loaders that were never initialized - #911
Open
bhuvan-somisetty wants to merge 1 commit into
Open
bhuvan-somisetty wants to merge 1 commit into
bhuvan-somisetty wants to merge 1 commit into
Conversation
loader_impl_destroy only calls the loader's destroy when it was initialized, and that destroy is what ends up calling loader_impl_destroy_objects. A loader that only got created (e.g. by calling metacall_execution_path without loading anything) skipped it, leaking handle_impl_path_map, handle_impl_map, handle_impl_init_order and type_info_map. Found by valgrind in metacall-path-overflow-test.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up to #910. With memcheck failing on leaks now,
metacall-path-overflow-testwas the only core test failing, with 176 bytes definitely lost fromloader_impl_allocate.The
ExecutionPathOverflowtest callsmetacall_execution_path("mock", ...)without loading anything, so the mock loader gets created but never initialized. Inloader_impl_destroythe loader's own destroy only runs wheninit == 0, and that's the one that ends up callingloader_impl_destroy_objectsthroughloader_unload_children. So for a loader that was never initialized,handle_impl_path_map,handle_impl_map,handle_impl_init_orderandtype_info_mapwere never freed. Now it callsloader_impl_destroy_objectsdirectly in that case. A loader that failed to initialize goes through the same path, so it's covered too.Tested locally:
MEMCHECK_TEST=metacall-path-overflow-test make memcheckgoes from 176 bytes definitely lost to 0 errors, fullmake memcheckpasses all 37 tests, and the ASan build passes all 41 tests.