> On Jan 27, 2026, at 04:55, Stefan Koch <[email protected]> wrote: > > In the KvpValue::Type::FRAME case if the guid matches, it does both: > delete inst->kvp_data->set_path({path}, nullptr); > delete v; > Where earlier the v was set to be: > auto v = inst->kvp_data->get_slot({path}); > I think the second delete is a duplicate of the first and should be removed. > > I found it by getting a strange warning and then a crash on that second > delete. I verified that it calls the frame destructor on the same object > twice. It is not surprising that double delete causes strange behavior, but I > did not verify the exact problem. > > The test case that crashed was: > auto gncGuid1 = guid_new(); > qof_instance_kvp_add_guid(m_inst, "guid", 123u, "mytime", gncGuid1); > qof_instance_kvp_remove_guid(m_inst, "guid", "mytime", gncGuid1); > > > This functionality is used in Split.cpp in support of > xaccScrubMergeLotSubSplits(). Where and why that is used, I am not sure, but > it is likely to be a problem under circumstances where the code is actually > called. I'm not sure if this is common, or even possible use case. > > I think the following will fix this issue: > modified libgnucash/engine/qofinstance.cpp > @@ -1216,7 +1216,6 @@ qof_instance_kvp_remove_guid (const QofInstance *inst, > const char *path, > if (kvp_match_guid (v, {key}, guid)) > { > delete inst->kvp_data->set_path({path}, nullptr); > - delete v; > } > break; > case KvpValue::Type::GLIST: > > > Since I am new, I wanted to get confirmation, that I am not missing something. > > If the change is good, I will add it (as a separate commit) to my next > testing merge request. That is what we agreed to on the previous, less > serious, issue I found.
Stefan, Yes, it’s a double free and removing the second delete is the correct fix. The GLIST branch of those functions was for backwards compatibility but I don’t think that I did it right. I don't think it’s reachable with current code unless you craft a test-only function to create a GLIST KVP that has a GUID element in it.. If that proves out then remove the switch and just test that the KVP value returned from the path is a KvpFrame*. Regards, John Ralls _______________________________________________ gnucash-devel mailing list [email protected] https://lists.gnucash.org/mailman/listinfo/gnucash-devel
