Fix: Add destructor to free strmap memory in sql plugin

[Andezion]

(Fixes #8802 )

Problem

When building with Clang and sanitizers enabled on Ubuntu 24.04, LeakSanitizer detects 1392 bytes of leaked memory in the sql plugin during init_tablemap():

Solution

Added a destroy_sql() destructor function that calls strmap_clear(&sql->tablemap) to properly free the internal strmap tree nodes. The destructor is registered with tal_add_destructor(sql, destroy_sql) in main(). Destructor to clean up strmap internal nodes.

Note: table_desc structures are tal-allocated as children of plugin context,
so they're freed automatically. This only cleans up the strmap tree overhead.

static void destroy_sql(struct sql *sql)
{
    strmap_clear(&sql->tablemap);
}

This ensures all memory is properly freed when the sql plugin terminates, resolving the LeakSanitizer errors.

Changelog-Fixed: Memory leak in sql plugin - freed strmap internal nodes on plugin cleanup (1392 bytes)

Important

26.04 FREEZE March 11th: Non-bugfix PRs not ready by this date will wait for 26.06.

RC1 is scheduled on March 23rd

The final release is scheduled for April 15th.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

• The changelog has been updated in the relevant commit(s) according to the guidelines.
• Tests have been added or modified to reflect the changes.
• Documentation has been reviewed and updated as needed.
• Related issues have been listed and linked, including any that this PR closes.
• Important All PRs must consider how to reverse any persistent changes for tools/lightning-downgrade

[rustyrussell]

Hi!

This fix is both correct, and insufficient.

1. Your first fix is fine, but it should be all one commit, so the Changelog-None is with the commit which actually does something. The reason it doesn't need a changelog is because it is not an unbounded leak.
2. It would be great if our CI actually broke when this happened. Why didn't it?
3. Turns out the complaint was about a different path (same leak though): we invoke the plugin to generate the schema for the documentation during the build. This path also needs strmap_clear, and is what the user is complaining about:

int main(int argc, char *argv[])
{
	struct sql *sql;
	setup_locale();

	if (argc == 2 && streq(argv[1], "--print-docs")) {
		tablemap tablemap;
		common_setup(argv[0]);

		/* plugin is NULL, so just sets up tables */
		init_tablemap(NULL, &tablemap);

		printf("The following tables are currently supported:\n");
		strmap_iterate(&tablemap, print_one_table, NULL);
		common_shutdown();
		return 0;
	}
...

[nepet]

Is this destructor ever being called, or is this just dead? If I understand it correctly, it only fires on tal_free during the processes lifetime. Calling rgrep "tal_free(sql" doesn't show a single line.

If this is the case, I'd drop it and only keep line :2216 which fixes the underlying issue.

[Andezion]

it runs on every normal plugin shutdown, just indirectly - sql is passed as take(sql) into plugin_main, which tal_steals it onto plugin (making it a tal child, not a standalone alloc). When lightningd stops the plugin, io_break fires and plugin_main calls tal_free(plugin) - freeing plugins children too, including sql, which triggers destroy_sql and clears tablemap. that's why grepping for tal_free(sql) finds nothing - the free happens via the parent (plugin), never by name. So the destructor should stay and line 2216 is an unrelated fix for the separate --print-docs early-exit path

[nepet]

I'm still not convinced that the tal_free(pluign) line below the for loop is ever getting called 😅

# plugins/libplugin.c - pugin_main()

	for (;;) {
		struct timer *expired = NULL;

		clean_tmpctx();

		/* Will only exit if a timer has expired. */
		io_loop(&plugin->timers, &expired);
		call_plugin_timer(plugin, expired);
	}
	tal_free(plugin);

However, it's not adding any harm, and seems idiomatically correct so let's just keep it!

[nepet]

ack