Fix memleaks in module.c #1602
Open
Fix memleaks in module.c #1602
Conversation
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
mat-hek
force-pushed
the
upstream-fix-memleaks
branch
from
433ac33 to
a177f52
Compare
This fixes some of the memleaks found by ASAN on CI. See https://github.com/software-mansion-labs/FissionVM/tree/ci-asan These changes are made under both the "Apache 2.0" and the "GNU Lesser General Public License 2.1 or later" license terms (dual license). SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later Signed-off-by: Mateusz Front <[email protected]>
mat-hek
force-pushed
the
upstream-fix-memleaks
branch
from
a177f52 to
caa6ee8
Compare
bettio
reviewed
Mar 28, 2025
| free(module->imported_funcs); | ||
| free(module->literals_table); | ||
| free(module->local_atoms_to_global_table); | ||
| if (module->free_literals_data) { | ||
| free(module->literals_data); | ||
| } | ||
| free(module->line_refs); | ||
| free(module->filenames); |
There was a problem hiding this comment.
I think this makes sense only in main since filenames is not available in release-0.6 branch.
The correct fix would be splitting this PR in 2, one for release-0.6 and one for main, but:
this memory leak for now is relevant only during tests so it doesn't have any practical impact on devices running release-0.6, so we can go for the lazy option that is basing this on top of main.
bettio
requested changes
Mar 28, 2025
pguyot
reviewed
Mar 29, 2025
| @@ -116,6 +116,7 @@ struct Module | |||
| struct ListHead line_ref_offsets; | |||
|
|
|||
| const struct ExportedFunction **imported_funcs; | |||
| int functions_count; | |||
There was a problem hiding this comment.
For the sake of saving sizeof(int) for every module, I would argue for recomputing this when the module is destroyed, especially considering we don't destroy modules at all for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes some of the memleaks found by ASAN running tests on Ubuntu. I believe having ASAN enabled on the AtomVM's CI would be beneficial, as there are still some memleaks out there
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later