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 a memleak in snowball compiler #166

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

jsteemann
Copy link
Contributor

When an output file is used and the name option is not, then the
compiler will dynamically allocate value for the name option and never
free it.
This is not a large problem, because at the end of the compile process
the OS will free all allocated memory anyway.
However, when using snowball as part of a larger toolchain and then
using compile options such as -fsanitize=leak, a memleak in snowball
can break the entire build.
This exactly what happened to us. We could work around this somehow, but
it seems better to fix the leak in the compiler properly.

The same issue was already reported previously by a former colleague of
mine in #136, but that PR
seemingly was disliked by the snowball maintainers. Trying it again with
this PR with a slightly modified solution and hope it can be merged now.

When an output file is used and the name option is not, then the
compiler will dynamically allocate value for the `name` option and never
free it.
This is not a large problem, because at the end of the compile process
the OS will free all allocated memory anyway.
However, when using snowball as part of a larger toolchain and then
using compile options such as `-fsanitize=leak`, a memleak in snowball
can break the entire build.
This exactly what happened to us. We could work around this somehow, but
it seems better to fix the leak in the compiler properly.

The same issue was already reported previously by a former colleague of
mine in snowballstem#136, but that PR
seemingly was disliked by the snowball maintainers. Trying it again with
this PR with a slightly modified solution and hope it can be merged now.
@jsteemann
Copy link
Contributor Author

2 out of 18 Travis CI sub-jobs failed during git clone. This is likely due to Github's outage yesterday.

jsteemann added a commit to arangodb/arangodb that referenced this pull request Mar 23, 2022
This fixes a memleak in the snowball compiler during generation of the
language-specific C files. The memleak currently makes our
ASan-/LSan-instrumented builds fail.

The same fix was suggested to upstream snowball via
snowballstem/snowball#166
but is still waiting on feedback there.
@jsteemann
Copy link
Contributor Author

Is there a chance this PR can make it into a future version of snowball?

@jsteemann
Copy link
Contributor Author

Any chance this PR can make it into a future version of snowball?

@ojwb ojwb mentioned this pull request Nov 8, 2022
@ojwb
Copy link
Member

ojwb commented Nov 8, 2022

I'll merge the patch, but I'd really strongly recommend using LSAN_OPTIONS=leak_check_at_exit=0 when running snowball to avoid this sort of problem (as I suggested in the other ticket).

Doing that would address the issue for you once and for all; otherwise next time the code changes such that something is allocated and not released on exit you'll have to spend time submitting another patch and I'll have to spend time reviewing and applying it.

@ojwb ojwb merged commit 9768dad into snowballstem:master Nov 8, 2022
ojwb added a commit that referenced this pull request Apr 24, 2023
@ojwb
Copy link
Member

ojwb commented Apr 24, 2023

This patch introduce a bogus -1 blocks unfreed message at the end of every run, fixed in 4af181b.

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.

2 participants