Fix many memory leaks, and add an address sanitizer check to CI #63
Conversation
This file contains 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
In cases where we sent a response but it was not awaited by the Glk thread, the response GVariant would be leaked. So instead, free the response GVariant when we free the UI message.
Forgot to allocate space for the terminal zero.
gtk_text_tag_table_add() adds a reference to the text tags added to the buffer, so we were leaking them here. (Note that we do still need to copy the text tag object, it needs to be distinct from the tags in the table where it is copied from.)
Occurred in ui_textwin_set_zcolors() and ui_textwin_set_reverse_video().
Overwriting the passed-in GdkRGBA out-args unconditionally can leave the calling function with nulls, if no colors were set in the cascade. Since we pass in GdkRGBAs with default values, I'm pretty sure this was not intentional.
I somehow never realized you had to unref this, but it makes sense!
This should be helpful in debugging a GNode memory leak in window closing. In order for the Glk thread not to hang on a sync-arrange UI message, we have to put the widget into an offscreen window.
When closing a non-root window, its GNode would get freed along with the parent pair window's GNode. But the root window's GNode would never get freed. Fix this with a different invocation of window_close_common for the root window.
When using chimara_glk_force_line_input() for line input, the input text would be pushed onto a queue. Then when popped off the queue (in several places), the text would never be freed.
In glk_image_get_info(), glk_image_draw(), and glk_image_draw_scaled(), we'd check if an image of the correct size was already cached, but would not free the lookup specifier. We can allocate the lookup specifier on the stack anyway, instead of the heap, so we don't need to worry about freeing it.
If reading a stream of byte data from a file into a Unicode buffer, we first need to read the bytes into a temporary byte buffer, then copy each element into a 32-bit element in the buffer we return to the Glk program. Previously, this temporary buffer would not be freed.
Previously, we would leak the text pasted into the text buffer window.
Occasionally this handler would be called after the window widget was already freed, so apparently the adjustment object lives longer than the window widget. To avoid this, disconnect the signal handler from the adjustment when the window is closed.
This is an unfortunate hack that _happens_ to make Glulxe work correctly when restarted without unloading the glulxe.so plugin. (The problem is in init_dispatch() which is not run a second time if it's already allocated the static data once, but that's where the object registry is set up.) This is needed in order to run things under the address sanitizer, because otherwise it will identify Glulxe's static data as "leaks", which normally would match the lines in the suppressions file, but without a stack trace (because the module is unloaded) they cannot match.
Both GLib and Glulxe allocate some static data. These allocations are not memory leaks, because they remain in memory throughout the lifetime of the process. The suppression in prepare_glk_args() is unfortunate because that function uses a scratch buffer which it frees only as needed when overwriting it with a new allocation. However, we can't get any more granular with the suppression rules, so there may be other memory leaks in that function which aren't caught. perform_saveundo() leaks on the Ubuntu image we use in CI for some reason, but I can't reproduce it on my system.
With all of these fixes, it should be possible to run address sanitizer in the automated tests, with the goal of catching memory leaks in every pull request. This requires some workarounds in Docker because it seems that the available debuginfo package repo for GitHub Actions' Ubuntu 22.04 image doesn't match exactly the versions available in the main package repo.
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.
I discovered many memory leaks by running the test suite with Address Sanitizer. Especially the new Glulxercise automated tests help to exercise a lot of the parts of the code that previously didn't get run during the tests. This PR patches all the memory leaks I discovered, and adds a new CI job that will fail if any new memory leaks are created in any code that is exercised as part of the test suite.