From 0e3853bfb73cf0edf276ce98ac74523c230b2a38 Mon Sep 17 00:00:00 2001 From: Stefan Koch Date: Thu, 15 Jan 2026 07:10:20 -0500 Subject: [PATCH] Fix undefined behaviour in collection_compare_cb function. The collection_compare_cb function set the user_data pointer to point to a local variable of the function. That pointer is then used in the (only) calling function qof_collection_compare to make decisions. This is undefined. It is likely not an actual problem because the stack depth of the qof_collection_foreach followed by collection_compare_cb is deeper than the qof_collection_get_data call (and others in between if any) that the stack data user data stays uncorrupted. But, it is undefined behavior, and could cause really subtle bugs if these there are code changes that have deeper stack between the setting and using. Also, using this local variable is not necessary, the qof_collection_compare function already sets up a variable local to its scope for this that the collection_compare_cb can use directly. This commit removes the local (to collection_compare_cb) variable and uses the one setup in qof_collection_compare. The full coverage test for qofid.cpp passed before and after this change. --- libgnucash/engine/qofid.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/libgnucash/engine/qofid.cpp b/libgnucash/engine/qofid.cpp index b1bf7e8e50..3e47ff9158 100644 --- a/libgnucash/engine/qofid.cpp +++ b/libgnucash/engine/qofid.cpp @@ -139,7 +139,7 @@ collection_compare_cb (QofInstance *ent, gpointer user_data) QofCollection *target; QofInstance *e; const GncGUID *guid; - gint value; + gint* value; e = NULL; target = (QofCollection*)user_data; @@ -147,28 +147,25 @@ collection_compare_cb (QofInstance *ent, gpointer user_data) { return; } - value = *(gint*)qof_collection_get_data(target); - if (value != 0) + value = (gint*)qof_collection_get_data(target); + if (*value != 0) { return; } guid = qof_instance_get_guid(ent); if (guid_equal(guid, guid_null())) { - value = -1; - qof_collection_set_data(target, &value); + *value = -1; return; } g_return_if_fail (target->e_type == ent->e_type); e = qof_collection_lookup_entity(target, guid); if ( e == NULL ) { - value = 1; - qof_collection_set_data(target, &value); + *value = 1; return; } - value = 0; - qof_collection_set_data(target, &value); + *value = 0; } gint