Inside qofinstance.cpp, the guid functionality is only used inside the following four functions. Those only seem to be used by the Split.cpp (in particular Peer splits.) The add_guid only handle the kvp frame not the list. The others work with either.
1. qof_instance_kvp_add_guid --- only used in split: xaccSplitAddPeerSplit Only does kvpframe version. 2. qof_instance_kvp_has_guid --- Only used in split: xaccSplitIsPeerSplit 3. qof_instance_kvp_remove_guid --- Only used in split: xaccSplitRemovePeerSplit 4. qof_instance_kvp_merge_guids --- Only used in xaccSplitMergePeerSplits I have removed the glist parts of the other three methods, and run the full test suite. There were no failed tests. I'm glad you suggested removing the glist code. I was kinda stuck on setting up an object manually just to test the other functions. It is not worth it, unless there is some concern that some real code is doing that. (I'm not sure I know how to look for that, so have not.) I will add the glist removal to the pending pull request (after I get the rest of qofinstance.cpp tested). On Tue, Jan 27, 2026 at 1:18 PM John Ralls <[email protected]> wrote: > > > > 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
