From e17ba3cc002bb19f1eb6a966462ce83bb3fa1959 Mon Sep 17 00:00:00 2001 From: John Ralls Date: Sun, 22 Oct 2023 10:15:44 -0700 Subject: [PATCH] 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. --- bindings/guile/gnc-optiondb.i | 4 ++-- libgnucash/backend/dbi/gnc-backend-dbi.cpp | 4 ++-- libgnucash/backend/xml/gnc-xml-backend.cpp | 7 ++++-- libgnucash/engine/Split.c | 11 +++++++--- libgnucash/engine/gnc-date.cpp | 7 ++++-- libgnucash/engine/gnc-timezone.cpp | 4 ++-- libgnucash/engine/qofbook.cpp | 25 +++++++++++++++++----- libgnucash/engine/test/test-guid.cpp | 2 +- libgnucash/engine/test/test-qofbook.c | 3 --- libgnucash/engine/test/test-qofobject.c | 6 +++--- 10 files changed, 48 insertions(+), 25 deletions(-) diff --git a/bindings/guile/gnc-optiondb.i b/bindings/guile/gnc-optiondb.i index 3340e24637..c2ac2e432a 100644 --- a/bindings/guile/gnc-optiondb.i +++ b/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 diff --git a/libgnucash/backend/dbi/gnc-backend-dbi.cpp b/libgnucash/backend/dbi/gnc-backend-dbi.cpp index 1235ad1b9f..f4b83b4117 100644 --- a/libgnucash/backend/dbi/gnc-backend-dbi.cpp +++ b/libgnucash/backend/dbi/gnc-backend-dbi.cpp @@ -1030,7 +1030,7 @@ template<> bool QofDbiBackendProvider::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::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) { diff --git a/libgnucash/backend/xml/gnc-xml-backend.cpp b/libgnucash/backend/xml/gnc-xml-backend.cpp index 7894d02f57..6f48e574f1 100644 --- a/libgnucash/backend/xml/gnc-xml-backend.cpp +++ b/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)) diff --git a/libgnucash/engine/Split.c b/libgnucash/engine/Split.c index 252babe507..424ae16a9a 100644 --- a/libgnucash/engine/Split.c +++ b/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); } diff --git a/libgnucash/engine/gnc-date.cpp b/libgnucash/engine/gnc-date.cpp index 3dc541313d..e7f0d49aea 100644 --- a/libgnucash/engine/gnc-date.cpp +++ b/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(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(df)}; + if (dfi >= DATE_FORMAT_FIRST && dfi <= DATE_FORMAT_LAST) { prevQofDateFormat = dateFormat; dateFormat = df; diff --git a/libgnucash/engine/gnc-timezone.cpp b/libgnucash/engine/gnc-timezone.cpp index b1db4769b4..5584e46e51 100644 --- a/libgnucash/engine/gnc-timezone.cpp +++ b/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)}); } } diff --git a/libgnucash/engine/qofbook.cpp b/libgnucash/engine/qofbook.cpp index 13bdb71f5d..7575f7a72f 100644 --- a/libgnucash/engine/qofbook.cpp +++ b/libgnucash/engine/qofbook.cpp @@ -32,6 +32,7 @@ * Copyright (c) 2000 Dave Peticolas * Copyright (c) 2007 David Hampton */ +#include "qof-string-cache.h" #include #include @@ -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 diff --git a/libgnucash/engine/test/test-guid.cpp b/libgnucash/engine/test/test-guid.cpp index 2f14d2bc66..78a4e5c4f6 100644 --- a/libgnucash/engine/test/test-guid.cpp +++ b/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"); diff --git a/libgnucash/engine/test/test-qofbook.c b/libgnucash/engine/test/test-qofbook.c index f131bcbb4c..e74d37a7e7 100644 --- a/libgnucash/engine/test/test-qofbook.c +++ b/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 diff --git a/libgnucash/engine/test/test-qofobject.c b/libgnucash/engine/test/test-qofobject.c index fae1379587..75d41c452a 100644 --- a/libgnucash/engine/test/test-qofobject.c +++ b/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