Possible bug in qofinstance.cpp: qof_instance_kvp_remove_guid

[John Ralls]

> On Jan 27, 2026, at 04:55, Stefan Koch <stefan.koch.micro at gmail.com> 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