Gnc commodity cpp

[christopherlam]

tested via add/remove custom commodity in ui

[jralls]

Code mostly looks OK except for the last commit, see below.

The first commit is dependent on the following two so it will cause crashes in bisects. The simplest fix would be to reorder it to after them but it would be more correct to combine the g_list_free changes with the respective conversion commit.
The two "use stl" commit messages are incorrect as they replace one STL algo (for_each) with a different one (all_of). Please fix them to say that.

The last commit is wrong. ns->name (not ns->mnemonic as claimed in the commit message) is CACHE_INSERTed and does need to be CACHE_REMOVEd.
The ASAN fail (below) is a use-after-free:

==11961==ERROR: AddressSanitizer: heap-use-after-free on address 0x6020000528b0 at pc 0x7fb7844925b3 bp 0x7fffa61efb80 sp 0x7fffa61ef328
READ of size 1 at 0x6020000528b0 thread T0
    #0 0x7fb7844925b2 in __interceptor_strcmp ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:466
    #1 0x7fb778da68a8 in gnc_commodity_table_map_namespace /home/runner/work/gnucash/gnucash/libgnucash/engine/gnc-commodity.cpp:1588
    #2 0x7fb778dabc76 in gnc_commodity_table_find_namespace /home/runner/work/gnucash/gnucash/libgnucash/engine/gnc-commodity.cpp:2123
    #3 0x7fb778dac1bc in gnc_commodity_table_delete_namespace /home/runner/work/gnucash/gnucash/libgnucash/engine/gnc-commodity.cpp:2152
    #4 0x7fb778dacfab in gnc_commodity_table_destroy /home/runner/work/gnucash/gnucash/libgnucash/engine/gnc-commodity.cpp:2202

0x6020000528b0 is located 0 bytes inside of 5-byte region [0x6020000528b0,0x6020000528b5)
freed by thread T0 here:
    #0 0x7fb7844b4537 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:127
    #1 0x7fb77b4ed3b8  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x383b8)
    #2 0x7fb77b4f5956 in g_hash_table_remove (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x40956)
    #3 0x7fb77932baa0 in qof_string_cache_remove /home/runner/work/gnucash/gnucash/libgnucash/engine/qof-string-cache.cpp:94
    #4 0x7fb778dac7e9 in gnc_commodity_table_delete_namespace /home/runner/work/gnucash/gnucash/libgnucash/engine/gnc-commodity.cpp:2163
    #5 0x7fb778dacfab in gnc_commodity_table_destroy /home/runner/work/gnucash/gnucash/libgnucash/engine/gnc-commodity.cpp:2202

Line 2202 is

for (auto ns : t->ns_vec)
        gnc_commodity_table_delete_namespace(t, ns->name);

The problem is that for (auto ns: t->ns_vec) is syntactic sugar for for(auto it = ns_vec.begin(); it != ns_vec.end(); ++it) auto ns = *it;. gnc_commodity_table_delete_namespace calls table->ns_vec.erase (std::remove (table->ns_vec.begin(), table->ns_vec.end(), ns));, invalidating it and ns_vec.end(). That causes two problems: ns_vec doesn't get completely cleared out and for (auto ns : t->ns_vec) will run off the end of the shrunken ns_vec causing use-after-free errors. As a band-aid you could instead just reverse that outer loop:

for (auto ns = t->ns_vec.rbegin(); ns != ns_vec.rend(); ++ns)
  gnc_commodity_table_delete_namespace(t, ns->name);

But consider the stupidity of the algorithm: We have an iterator in the vector. We get the string name from that iterator and search the vector again with the string to find the iterator that we already have, then convert the iterator to its object and search the vector yet again to remove the iterator. Then we destroy the object's members (ignoring its ref count and not NULLing any of them) and finally call the object's destructor (g_object_unref(ns)) which decrements the ref count and frees the memory assuming nothing has reffed it anywhere.

If the object cleanup was in gnc_commodity_namespace_dispose_real where it belongs then gnc_commodity_table_delete_namespace could just find the object from the name and g_object_unref it, and the table destructor could do

   while(!ns_vec.empty())
       g_object_unref(ns_vec.back());

Unfortunately gnc_commodity_namespace is widely used in C code so a C++ class would need C wrapper functions, but it doesn't use any of the features of QofInstance; it perhaps should use GObject ref counting, but considering how rare it is to do so in any GnuCash code it probably doesn't.

[christopherlam]

The first commit is dependent on the following two so it will cause crashes in bisects

The first commit is one that makes the get_commodities and get_namespaces return GLists that must be freed by the caller, so I was actually thinking best to merge it separately first, so that this branch is purely c++ refactoring.

[jralls]

The first commit is one that makes the get_commodities and get_namespaces return GLists that must be freed by the caller

Ah, missed the g_list_copys. OK.

[christopherlam]

I see the silly object cleanup code paths... and hopefully with the lesser LOC here it would be easier to simplify.