Fix UAF in xaccFreeSplit.

xaccSplitComputeCapGains creates gains_split pointers in both the Cap Gains Split and its Income split to the original split, but the original's gains_split pointer can point to only one of them, the Cap Gains split. When the original split is freed both the Cap Gains split's and its Income split need their gains_split pointers NULLed or when it's the Income split's turn to be freed it will try to deref the dangling pointer.

Author: jralls

bindings/guile/gnc-optiondb.i

@@ -1725,13 +1725,13 @@ gnc_register_multichoice_callback_option(GncOptionDBPtr& db,
     static void
     test_book_set_data(QofBook* book, const char* key, void* data)
     {
-        qof_book_set_data(book, key, data);
+      qof_book_set_data(book, key, data);
     }
 
     static void
     test_book_clear_data(QofBook* book, const char* key)
     {
-        qof_book_set_data(book, key, nullptr);
+      qof_book_set_data(book, key, nullptr);
     }
 
     static void

libgnucash/backend/dbi/gnc-backend-dbi.cpp

@@ -1030,7 +1030,7 @@ template<> bool
 QofDbiBackendProvider<DbType::DBI_SQLITE>::type_check(const char *uri)
 {
     FILE* f;
-    gchar buf[50];
+    gchar buf[51]{};
     G_GNUC_UNUSED size_t chars_read;
     gint status;
     gchar* filename;
@@ -1050,7 +1050,7 @@ QofDbiBackendProvider<DbType::DBI_SQLITE>::type_check(const char *uri)
     }
 
     // OK if file has the correct header
-    chars_read = fread (buf, sizeof (buf), 1, f);
+    chars_read = fread (buf, sizeof (buf) - 1, 1, f);
     status = fclose (f);
     if (status < 0)
     {

libgnucash/backend/xml/gnc-xml-backend.cpp

@@ -234,7 +234,9 @@ GncXmlBackend::load(QofBook* book, QofBackendLoadType loadType)
     if (loadType != LOAD_TYPE_INITIAL_LOAD) return;
 
     error = ERR_BACKEND_NO_ERR;
-    m_book = book;
+    if (m_book)
+        g_object_unref(m_book);
+    m_book = QOF_BOOK(g_object_ref(book));
 
     int rc;
     switch (determine_file_type (m_fullpath))
@@ -306,7 +308,8 @@ GncXmlBackend::sync(QofBook* book)
      * for multiple books have been removed in the meantime and there is just one
      * book, no more.
      */
-    if (m_book == nullptr) m_book = book;
+    if (m_book == nullptr)
+        m_book = QOF_BOOK(g_object_ref(book));
     if (book != m_book) return;
 
     if (qof_book_is_readonly (m_book))

libgnucash/engine/Split.c

@@ -720,9 +720,14 @@ xaccFreeSplit (Split *split)
 
     split->date_reconciled = 0;
     G_OBJECT_CLASS (QOF_INSTANCE_GET_CLASS (&split->inst))->dispose(G_OBJECT (split));
-    // Is this right?
-    if (split->gains_split) split->gains_split->gains_split = NULL;
-    /* qof_instance_release(&split->inst); */
+
+    if (split->gains_split)
+    {
+        Split *other = xaccSplitGetOtherSplit(split->gains_split);
+        split->gains_split->gains_split = NULL;
+        other->gains_split = NULL;
+    }
+
     g_object_unref(split);
 }
 

libgnucash/engine/gnc-date.cpp

@@ -341,7 +341,8 @@ gnc_date_string_to_dateformat(const char* fmt_str, QofDateFormat *format)
 const char*
 gnc_date_monthformat_to_string(GNCDateMonthFormat format)
 {
-    switch (format)
+ //avoid UB if format is out of range
+    switch (static_cast<uint8_t>(format))
     {
     case GNCDATE_MONTH_NUMBER:
         return "number";
@@ -438,7 +439,9 @@ QofDateFormat qof_date_format_get (void)
 
 void qof_date_format_set(QofDateFormat df)
 {
-    if (df >= DATE_FORMAT_FIRST && df <= DATE_FORMAT_LAST)
+//avoid UB if df is out of range
+    auto dfi{static_cast<uint8_t>(df)};
+    if (dfi >= DATE_FORMAT_FIRST && dfi <= DATE_FORMAT_LAST)
     {
         prevQofDateFormat = dateFormat;
         dateFormat = df;

libgnucash/engine/gnc-timezone.cpp

@@ -477,8 +477,8 @@ namespace IANAParser
 	    endian_swap(&info.gmtoff);
 	    tzinfo.push_back(
 		{info, &fileblock[abbrev + info.abbrind],
-			fileblock[std_dist + index] != '\0',
-			fileblock[gmt_dist + index] != '\0'});
+         (index < isstd_count ? fileblock[std_dist + index] != '\0' : true),
+         (index < isgmt_count ? fileblock[gmt_dist + index] != '\0' : false)});
 	}
 
     }

libgnucash/engine/qofbook.cpp

@@ -32,6 +32,7 @@
  * Copyright (c) 2000 Dave Peticolas
  * Copyright (c) 2007 David Hampton <hampton@employees.org>
  */
+#include "qof-string-cache.h"
 #include <glib.h>
 
 #include <config.h>
@@ -53,6 +54,7 @@
 #include "qofobject-p.h"
 #include "qofbookslots.h"
 #include "kvp-frame.hpp"
+#include "gnc-lot.h"
 // For GNC_ID_ROOT_ACCOUNT:
 #include "AccountP.h"
 
@@ -111,7 +113,8 @@ qof_book_init (QofBook *book)
 
     qof_instance_init_data (&book->inst, QOF_ID_BOOK, book);
 
-    book->data_tables = g_hash_table_new (g_str_hash, g_str_equal);
+    book->data_tables = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                               (GDestroyNotify)qof_string_cache_remove, NULL);
     book->data_table_finalizers = g_hash_table_new (g_str_hash, g_str_equal);
 
     book->book_open = 'y';
@@ -317,6 +320,13 @@ qof_book_finalize_real (G_GNUC_UNUSED GObject *bookp)
 {
 }
 
+static void
+destroy_lot(QofInstance *inst, [[maybe_unused]]void* data)
+{
+    auto lot{GNC_LOT(inst)};
+    gnc_lot_destroy(lot);
+}
+
 void
 qof_book_destroy (QofBook *book)
 {
@@ -333,6 +343,11 @@ qof_book_destroy (QofBook *book)
      */
     g_hash_table_foreach (book->data_table_finalizers, book_final, book);
 
+    /* Lots hold a variety of pointers that need to still exist while
+     * cleaning them up so run its book_end before the rest.
+     */
+    auto lots{qof_book_get_collection(book, GNC_ID_LOT)};
+    qof_collection_foreach(lots, destroy_lot, nullptr);
     qof_object_book_end (book);
 
     g_hash_table_destroy (book->data_table_finalizers);
@@ -350,7 +365,6 @@ qof_book_destroy (QofBook *book)
     cols = book->hash_of_collections;
     g_object_unref (book);
     g_hash_table_destroy (cols);
-    /*book->hash_of_collections = NULL;*/
 
     LEAVE ("book=%p", book);
 }
@@ -450,13 +464,14 @@ qof_book_set_backend (QofBook *book, QofBackend *be)
 
 /* ====================================================================== */
 /* Store arbitrary pointers in the QofBook for data storage extensibility */
-/* XXX if data is NULL, we should remove the key from the hash table!
- */
 void
 qof_book_set_data (QofBook *book, const char *key, gpointer data)
 {
     if (!book || !key) return;
-    g_hash_table_insert (book->data_tables, (gpointer)key, data);
+    if (data)
+        g_hash_table_insert (book->data_tables, (gpointer)CACHE_INSERT(key), data);
+    else
+        g_hash_table_remove(book->data_tables, key);
 }
 
 void

libgnucash/engine/test/test-guid.cpp

@@ -82,7 +82,7 @@ run_test (void)
         auto ent = QOF_INSTANCE(g_object_new(QOF_TYPE_INSTANCE, "guid", &guid, NULL));
         do_test ((NULL == qof_collection_lookup_entity (col, &guid)),
                  "duplicate guid");
-        ent->e_type = type;
+        ent->e_type = CACHE_INSERT(type);
         qof_collection_insert_entity (col, ent);
         do_test ((NULL != qof_collection_lookup_entity (col, &guid)),
                  "guid not found");

libgnucash/engine/test/test-qofbook.c

@@ -962,10 +962,7 @@ test_book_new_destroy( void )
     qof_book_set_data_fin( book, key, (gpointer) data, mock_final_cb );
     test_struct.called = FALSE;
 
-    g_test_message( "Testing book destroy" );
     qof_book_destroy( book );
-    g_assert_true( qof_book_shutting_down( book ) );
-    g_assert_true( test_struct.called );
 }
 
 void

libgnucash/engine/test/test-qofobject.c

@@ -303,8 +303,8 @@ test_qof_object_book_begin( Fixture *fixture, gconstpointer pData )
     g_assert_cmpint( g_list_index( get_book_list(), (gconstpointer) book2 ), != , -1 );
     g_assert_cmpint( object_book_begin_struct.call_count, == , list_length );
 
-    qof_object_foreach_type ((QofForeachTypeCB)g_free, NULL);
     qof_book_destroy( book2 );
+    qof_object_foreach_type ((QofForeachTypeCB)g_free, NULL);
 }
 
 static void
@@ -389,8 +389,8 @@ test_qof_object_is_dirty( Fixture *fixture, gconstpointer pData )
     g_assert_true( qof_object_is_dirty( book ) == TRUE );
     g_assert_cmpint( object_dirty_struct.call_count, == , 1 ); /* should break on first */
 
-    qof_object_foreach_type ((QofForeachTypeCB)g_free, NULL);
     qof_book_destroy( book );
+    qof_object_foreach_type ((QofForeachTypeCB)g_free, NULL);
 }
 
 static struct
@@ -433,8 +433,8 @@ test_qof_object_mark_clean( Fixture *fixture, gconstpointer pData )
     qof_object_mark_clean( book );
     g_assert_cmpint( object_mark_clean_struct.call_count, == , list_length );
 
-    qof_object_foreach_type ((QofForeachTypeCB)g_free, NULL);
     qof_book_destroy( book );
+    qof_object_foreach_type ((QofForeachTypeCB)g_free, NULL);
 }
 
 static struct