Skip to content

Fix: Add destructor to free strmap memory in sql plugin #8827

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.

Sign up for GitHub

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

Open
Andezion wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix: Add destructor to free strmap memory in sql plugin #8827

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cbc5bb5
fix: add destructor to free strmap memory in sql plugin
Andezion Jan 7, 2026
08dfa5e
sql: free strmap internal nodes on plugin shutdown
Andezion Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions plugins/sql.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,11 @@ struct sql {
struct command *waitcmd;
};

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

static struct sql *sql_of(struct plugin *plugin)
{
return plugin_get_data(plugin, struct sql);
Expand Down Expand Up @@ -2208,11 +2213,13 @@ int main(int argc, char *argv[])

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

sql = tal(NULL, struct sql);
tal_add_destructor(sql, destroy_sql);

@nepet nepet Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Andezion Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nepet Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, so I'm fine to keep it.

sql->dbfilename = NULL;
sql->gosstore_fd = -1;
sql->gosstore_nodes_off = sql->gosstore_channels_off = 0;
Expand Down
Loading