Fix many memory leaks, and add an address sanitizer check to CI

.github/workflows/ci.yml

@@ -58,3 +58,61 @@ jobs:
       - name: print-test-log
         if: ${{ failure() }}
         run: cat _build/meson-logs/testlog.txt
+
+  sanitizer:
+    runs-on: ubuntu-22.04
+    steps:
+      - name: install-deps
+        # gtk downgrade is because there is no matching version in ddebs repo
+        # for the latest
+        run: |
+          sudo apt-get update
+          sudo apt-get -y --allow-downgrades install \
+            gir1.2-gtk-3.0=3.24.33-1ubuntu1 \
+            gstreamer1.0-plugins-bad \
+            gstreamer1.0-plugins-good \
+            gstreamer1.0-tools \
+            libc6-dbg \
+            libgstreamer1.0-dev \
+            libgtk-3-0=3.24.33-1ubuntu1 \
+            libgtk-3-dev=3.24.33-1ubuntu1 \
+            libunwind-dev \
+            libxml2-utils \
+            meson \
+            ubuntu-dbgsym-keyring
+
+      - name: install-debug-symbols
+        run: |
+          sudo tee -a "/etc/apt/sources.list.d/ddebs.list" <<EOF
+          deb http://ddebs.ubuntu.com jammy main restricted universe multiverse
+          deb http://ddebs.ubuntu.com jammy-updates main restricted universe multiverse
+          EOF
+          sudo apt-get update
+          sudo apt-get -y install libglib2.0-0-dbgsym libgtk-3-0-dbgsym
+
+      - uses: actions/checkout@v3
+
+      - name: configure
+        run: |
+          meson _build \
+            -Db_sanitize=address \
+            -Dintrospection=false \
+            -Dbocfel=false \
+            -Dfrotz=false \
+            -Dgit=false \
+            -Dnitfol=false \
+            -Dplayer=false
+
+      - name: build
+        run: ASAN_OPTIONS=detect_leaks=0 ninja -C _build
+
+      - name: test
+        run: |
+          G_DEBUG=gc-friendly \
+          G_SLICE=always-malloc \
+          ASAN_OPTIONS=fast_unwind_on_malloc=0 \
+            xvfb-run -a meson test -C _build
+
+      - name: print-test-log
+        if: ${{ failure() }}
+        run: cat _build/meson-logs/testlog.txt

libchimara/abort.c

@@ -153,9 +153,13 @@ shutdown_glk_post(void)
 	glk_data->interrupt_handler = NULL;
 	g_free(glk_data->current_dir);
 	glk_data->current_dir = NULL;
-	/* Remove the dispatch callbacks */
+	/* Remove the dispatch callbacks. However, if we are running under address
+	 * sanitizer, leave them in place - this is an unfortunate hack that allows
+	 * Glulxe to work twice in a row without unloading the plugin in between. */
+#ifndef CHIMARA_ASAN_HACK
 	glk_data->register_obj = NULL;
 	glk_data->unregister_obj = NULL;
 	glk_data->register_arr = NULL;
 	glk_data->unregister_arr = NULL;
+#endif
 }

libchimara/chimara-glk-private.h

@@ -11,6 +11,16 @@
 
 G_BEGIN_DECLS
 
+#ifdef __has_feature
+# if __has_feature(address_sanitizer)
+#  define CHIMARA_ASAN_HACK 1
+# endif
+#else
+# ifdef __SANITIZE_ADDRESS__
+#  define CHIMARA_ASAN_HACK 1
+# endif
+#endif
+
 typedef struct StyleSet {
 	GHashTable *text_grid;
 	GHashTable *text_buffer;

libchimara/chimara-glk.c

@@ -178,6 +178,9 @@ chimara_glk_reset_glk_styles(ChimaraGlk *self)
 		g_hash_table_insert(glk_text_buffer_styles, (char *) GLK_TAG_NAMES[i], tag);
 	}
 
+	g_clear_pointer(&priv->glk_styles->text_grid, g_hash_table_destroy);
+	g_clear_pointer(&priv->glk_styles->text_buffer, g_hash_table_destroy);
+
 	priv->glk_styles->text_grid = glk_text_grid_styles;
 	priv->glk_styles->text_buffer = glk_text_buffer_styles;
 }
@@ -353,7 +356,8 @@ text_tag_copy(GtkTextTag *tag)
 static void
 copy_tag_to_textbuffer(void *key, void *tag, void *target_table)
 {
-	gtk_text_tag_table_add(target_table, text_tag_copy(GTK_TEXT_TAG(tag)));
+	g_autoptr(GtkTextTag) tag_copy = text_tag_copy(GTK_TEXT_TAG(tag));
+	gtk_text_tag_table_add(GTK_TEXT_TAG_TABLE(target_table), tag_copy);
 }
 
 /* Private method: initialize the default styles to a textbuffer. */
@@ -544,6 +548,7 @@ chimara_glk_finalize(GObject *object)
 	g_free(priv->story_name);
 	g_free(priv->styles);
 	g_free(priv->glk_styles);
+	g_clear_pointer(&priv->thread, g_thread_unref);
 
 	/* Chain up to parent */
     G_OBJECT_CLASS(chimara_glk_parent_class)->finalize(object);
@@ -1465,8 +1470,16 @@ chimara_glk_run(ChimaraGlk *self, const gchar *plugin, int argc, char *argv[], G
 
     g_assert( g_module_supported() );
 	/* If there is already a module loaded, free it first -- you see, we want to
-	 * keep modules loaded as long as possible to avoid crashes in stack unwinding */
+	 * keep modules loaded as long as possible to avoid crashes in stack
+	 * unwinding. And in fact, skip unloading it altogether if we are running
+	 * under address sanitizer, because otherwise symbolizing stack traces at
+	 * the end of the process won't work (and then the stack traces won't match
+	 * the suppressions file. This is an unfortunate but necessary hack unless
+	 * we can either deallocate all static data in glulxe manually, or rewrite
+	 * the unit test suite so that it doesn't need to unload plugins. */
+#ifndef CHIMARA_ASAN_HACK
 	chimara_glk_unload_plugin(self);
+#endif
 	/* Open the module to run */
     priv->program = g_module_open(plugin, G_MODULE_BIND_LAZY);
 
@@ -1515,6 +1528,7 @@ chimara_glk_run(ChimaraGlk *self, const gchar *plugin, int argc, char *argv[], G
 	priv->ui_message_handler_id = gdk_threads_add_idle((GSourceFunc)chimara_glk_process_queue, self);
 
     /* Run in a separate thread */
+	g_clear_pointer(&priv->thread, g_thread_unref);
 	priv->thread = g_thread_try_new("glk", (GThreadFunc)glk_enter, startup, error);
 
 	return !(priv->thread == NULL);
@@ -1620,7 +1634,7 @@ chimara_glk_wait(ChimaraGlk *self)
 	/* Empty UI message queue first, so that the Glk thread isn't waiting on any
 	UI operations; then it's safe to wait for the Glk thread to finish */
 	chimara_glk_drain_queue(self);
-    g_thread_join(priv->thread);
+	g_clear_pointer(&priv->thread, g_thread_join);
 }
 
 /**

libchimara/fileref.c

@@ -345,8 +345,8 @@ glk_fileref_create_by_name(glui32 usage, char *name, glui32 rock)
 	/* Do any string-munging here to remove illegal Latin-1 characters from 
 	filename. On ext3, the only illegal characters are '/' and '\0', but the Glk
 	spec calls for removing any other tricky characters. */
-	char *buf = g_malloc(strlen(name));
-	char *ptr, *filename, *extension;
+	char *buf = g_malloc(strlen(name) + 1);
+	char *ptr, *extension;
 	int len;
 	for(ptr = name, len = 0; *ptr && *ptr != '.'; ptr++)
 	{
@@ -383,8 +383,8 @@ glk_fileref_create_by_name(glui32 usage, char *name, glui32 rock)
 			ILLEGAL_PARAM("Unknown file usage: %u", usage);
 			return NULL;
 	}
-	filename = g_strconcat(buf, extension, NULL);
-	
+	g_autofree char *filename = g_strconcat(buf, extension, NULL);
+
 	/* Find out what encoding filenames are in */
 	const gchar **charsets; /* Do not free */
 	g_get_filename_charsets(&charsets);

libchimara/graphics.c

@@ -240,12 +240,12 @@ image_cache_find(struct image_info* to_find)
 glui32
 glk_image_get_info(glui32 image, glui32 *width, glui32 *height)
 {
-	struct image_info *to_find = g_new0(struct image_info, 1);
+	struct image_info to_find = {
+		.resource_number = image,
+		.scaled = false,  /* we want the original image size */
+	};
 	struct image_info *found;
-	to_find->resource_number = image;
-	to_find->scaled = FALSE; /* we want the original image size */
-
-	if( !(found = image_cache_find(to_find)) ) {
+	if (!(found = image_cache_find(&to_find))) {
 		found = load_image_in_cache(image, 0, 0);
 		if(found == NULL)
 			return FALSE;
@@ -292,14 +292,14 @@ glk_image_draw(winid_t win, glui32 image, glsi32 val1, glsi32 val2)
 	VALID_WINDOW(win, return FALSE);
 	g_return_val_if_fail(win->type == wintype_Graphics || win->type == wintype_TextBuffer, FALSE);
 
-	struct image_info *to_find = g_new0(struct image_info, 1);
+	struct image_info to_find = {
+		.resource_number = image,
+		.scaled = false,  /* we want the original image size */
+	};
 	struct image_info *info;
 
 	/* Lookup the proper resource */
-	to_find->resource_number = image;
-	to_find->scaled = FALSE; /* we want the original image size */
-
-	if( !(info = image_cache_find(to_find)) ) {
+	if (!(info = image_cache_find(&to_find))) {
 		info = load_image_in_cache(image, 0, 0);
 		if(info == NULL)
 			return FALSE;
@@ -354,15 +354,16 @@ glk_image_draw_scaled(winid_t win, glui32 image, glsi32 val1, glsi32 val2, glui3
 	g_return_val_if_fail(width != 0 && height != 0, FALSE);
 
 	ChimaraGlkPrivate *glk_data = g_private_get(&glk_data_key);
-	struct image_info *to_find = g_new0(struct image_info, 1);
+
+	struct image_info to_find = {
+		.resource_number = image,
+		.scaled = true,  /* any image size equal or larger than requested will do */
+	};
 	struct image_info *info;
 	struct image_info *scaled_info;
 
 	/* Lookup the proper resource */
-	to_find->resource_number = image;
-	to_find->scaled = TRUE; /* any image size equal or larger than requested will do */
-
-	if( !(info = image_cache_find(to_find)) ) {
+	if (!(info = image_cache_find(&to_find))) {
 		info = load_image_in_cache(image, width, height);
 		if(info == NULL)
 			return FALSE;

libchimara/input.c

@@ -355,10 +355,9 @@ void
 force_line_input_from_queue(winid_t win, event_t *event)
 {
 	ChimaraGlkPrivate *glk_data = g_private_get(&glk_data_key);
-	const gchar *text = g_async_queue_pop(glk_data->line_input_queue);
 
 	UiMessage *msg = ui_message_new(UI_MESSAGE_FORCE_LINE_INPUT, win);
-	msg->strval = g_strdup(text);
+	msg->strval = g_async_queue_pop(glk_data->line_input_queue);
 	glui32 chars_written = ui_message_queue_and_await(msg);
 
 	event->type = evtype_LineInput;

libchimara/strio.c

@@ -1228,7 +1228,7 @@ glk_get_line_stream_uni(strid_t str, glui32 *buf, glui32 len)
 				}
 				else /* Regular binary file */
 				{
-					gchar *readbuffer = g_new0(gchar, len);
+					g_autofree char *readbuffer = g_new0(char, len);
 					ensure_file_operation(str, filemode_Read);
 					if( !fgets(readbuffer, len, str->file_pointer) ) {
 						*buf = 0;

libchimara/ui-buffer.c

@@ -201,6 +201,8 @@ ui_buffer_create(winid_t win, ChimaraGlk *glk)
 {
 	win->frame = gtk_overlay_new();
 	win->scrolledwindow = gtk_scrolled_window_new(NULL, NULL);
+	win->vadjustment = gtk_scrolled_window_get_vadjustment(GTK_SCROLLED_WINDOW(win->scrolledwindow));
+	g_object_add_weak_pointer(G_OBJECT(win->vadjustment), (void **)&win->vadjustment);
 	win->widget = gtk_text_view_new();
 	win->pager = gtk_button_new_with_label(_("More"));
 	GtkWidget *image = gtk_image_new_from_icon_name("go-down", GTK_ICON_SIZE_BUTTON);
@@ -237,8 +239,7 @@ ui_buffer_create(winid_t win, ChimaraGlk *glk)
 	g_signal_connect_after( win->widget, "size-allocate", G_CALLBACK(pager_after_size_allocate), win );
 	win->pager_keypress_handler = g_signal_connect( win->widget, "key-press-event", G_CALLBACK(pager_on_key_press_event), win );
 	g_signal_handler_block(win->widget, win->pager_keypress_handler);
-	GtkAdjustment *adj = gtk_scrolled_window_get_vadjustment( GTK_SCROLLED_WINDOW(win->scrolledwindow) );
-	win->pager_adjustment_handler = g_signal_connect_after(adj, "value-changed", G_CALLBACK(pager_after_adjustment_changed), win);
+	win->pager_adjustment_handler = g_signal_connect_after(win->vadjustment, "value-changed", G_CALLBACK(pager_after_adjustment_changed), win);
 	g_signal_connect(win->pager, "clicked", G_CALLBACK(pager_on_clicked), win);
 
 	/* Char and line input */
@@ -370,7 +371,7 @@ ui_buffer_force_line_input(winid_t win, const char *text)
 	/* Insert the forced input into the window */
 	if(win->echo_current_line_input) {
 		gtk_text_buffer_get_end_iter(buffer, &end);
-		char *text_to_insert = g_strconcat(text, "\n", NULL);
+		g_autofree char *text_to_insert = g_strconcat(text, "\n", NULL);
 		gtk_text_buffer_insert_with_tags_by_name(buffer, &end, text_to_insert, -1, "default", "input", "glk-input", NULL);
 	}
 

libchimara/ui-message.c

@@ -85,6 +85,7 @@ void
 ui_message_free(UiMessage *msg)
 {
 	g_free(msg->strval);
+	g_clear_pointer(&msg->response, g_variant_unref);
 	g_slice_free(UiMessage, msg);
 }
 
@@ -156,7 +157,6 @@ ui_message_queue_and_await(UiMessage *msg)
 {
 	queue_and_await_response(msg);
 	int retval = g_variant_get_int64(msg->response);
-	g_variant_unref(msg->response);
 	ui_message_free(msg);
 	return retval;
 }
@@ -174,7 +174,6 @@ ui_message_queue_and_await_string(UiMessage *msg)
 	queue_and_await_response(msg);
 	char *retval;
 	g_variant_get(msg->response, "ms", &retval);
-	g_variant_unref(msg->response);
 	ui_message_free(msg);
 	return retval;
 }
@@ -188,6 +187,7 @@ ui_message_respond(UiMessage *msg, gint64 response)
 {
 	g_mutex_lock(&msg->lock);
 	msg->response = g_variant_new_int64(response);
+	g_variant_ref_sink(msg->response);
 	g_cond_signal(&msg->sign);
 	g_mutex_unlock(&msg->lock);
 }
@@ -205,6 +205,7 @@ ui_message_respond_string(UiMessage *msg, char *response)
 	g_mutex_lock(&msg->lock);
 	msg->response = g_variant_new_maybe(G_VARIANT_TYPE_STRING,
 		response != NULL ? g_variant_new_take_string(response) : NULL);
+	g_variant_ref_sink(msg->response);
 	g_cond_signal(&msg->sign);
 	g_mutex_unlock(&msg->lock);
 }

libchimara/ui-style.c

@@ -62,8 +62,10 @@ style_cascade_colors(GtkTextTag *tag, GtkTextTag *glk_tag, GtkTextTag *default_t
 		g_object_get(glk_tag, "background-rgba", dest, NULL);
 	}
 
-	*foreground = fg;
-	*background = bg;
+	if (fg)
+		*foreground = fg;
+	if (bg)
+		*background = bg;
 }
 
 /* Internal function: changes a GTK tag to correspond with the given style. */

libchimara/ui-textwin.c

@@ -329,7 +329,7 @@ ui_textwin_set_zcolors(winid_t win, unsigned fg, unsigned bg)
 		// NULL value means to ignore the zcolor property altogether
 		win->zcolor = NULL;
 	} else {
-		char *name = g_strdup_printf(ZCOLOR_NAME_TEMPLATE, fore_name, back_name);
+		g_autofree char *name = g_strdup_printf(ZCOLOR_NAME_TEMPLATE, fore_name, back_name);
 		g_free(fore_name);
 		g_free(back_name);
 
@@ -389,7 +389,7 @@ ui_textwin_set_reverse_video(winid_t win, gboolean reverse)
 	// Name the color
 	char *foreground_name = gdk_rgba_to_string(current_foreground);
 	char *background_name = gdk_rgba_to_string(current_background);
-	char *name = g_strdup_printf(ZCOLOR_NAME_TEMPLATE, foreground_name, background_name);
+	g_autofree char *name = g_strdup_printf(ZCOLOR_NAME_TEMPLATE, foreground_name, background_name);
 	g_free(foreground_name);
 	g_free(background_name);
 

libchimara/window.c

@@ -83,6 +83,9 @@ window_close_common(winid_t win, gboolean destroy_node)
 	g_clear_object(&win->font_override);
 	g_clear_object(&win->background_override);
 
+	if (win->vadjustment)
+		g_clear_signal_handler(&win->pager_adjustment_handler, win->vadjustment);
+
 	g_mutex_clear(&win->lock);
 
 	g_slice_free(struct glk_window_struct, win);
@@ -743,13 +746,15 @@ glk_window_close(winid_t win, stream_result_t *result)
 
 		stream_close_common( ((winid_t) pair_node->data)->window_stream, NULL );
 		window_close_common( (winid_t) pair_node->data, TRUE);
-	} 
+		/* node is now already freed by pair_node */
+		window_close_common(win, /* free_node = */ FALSE);
+	}
 	else /* it was the root window */
 	{
 		glk_data->root_window = NULL;
+		window_close_common(win, TRUE);
 	}
 
-	window_close_common(win, FALSE);
 	g_mutex_unlock(&glk_data->arrange_lock);
 
 	/* Schedule a redraw */

libchimara/window.h

@@ -72,6 +72,7 @@ struct glk_window_struct
 	/* In text buffer windows, the scrolled window and the pager are extra
 	widgets that are neither "widget" nor "frame" */
 	GtkWidget *scrolledwindow;
+	GtkAdjustment *vadjustment;  /* unowned weak pointer */
 	GtkWidget *pager;
 	/* Input request stuff */
 	enum InputRequestType input_request_type;