Avoid unnecessary memory allocation in dom_tree_to_gnc_numeric()

Return a gnc_numeric instead of allocations that every caller has to free.

This makes it easier to fix the use after free in the unit test function
equals_node_val_vs_split_internal() where the expression in the return
statement wants to use the allocated gnc_numeric.

libgnucash/backend/xml/gnc-bill-term-xml-v2.cpp

@@ -151,11 +151,7 @@ static gboolean
 set_numeric (xmlNodePtr node, GncBillTerm* term,
              void (*func) (GncBillTerm*, gnc_numeric))
 {
-    gnc_numeric* num = dom_tree_to_gnc_numeric (node);
+    func (term, dom_tree_to_gnc_numeric (node));
-    g_return_val_if_fail (num, FALSE);
-
-    func (term, *num);
-    g_free (num);
     return TRUE;
 }
 

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

@@ -289,14 +289,8 @@ static gboolean
 customer_discount_handler (xmlNodePtr node, gpointer cust_pdata)
 {
     struct customer_pdata* pdata = static_cast<decltype (pdata)> (cust_pdata);
-    gnc_numeric* val;
-
-    val = dom_tree_to_gnc_numeric (node);
-    g_return_val_if_fail (val, FALSE);
-
-    gncCustomerSetDiscount (pdata->customer, *val);
-    g_free (val);
 
+    gncCustomerSetDiscount (pdata->customer, dom_tree_to_gnc_numeric (node));
     return TRUE;
 }
 
@@ -304,14 +298,8 @@ static gboolean
 customer_credit_handler (xmlNodePtr node, gpointer cust_pdata)
 {
     struct customer_pdata* pdata = static_cast<decltype (pdata)> (cust_pdata);
-    gnc_numeric* val;
-
-    val = dom_tree_to_gnc_numeric (node);
-    g_return_val_if_fail (val, FALSE);
-
-    gncCustomerSetCredit (pdata->customer, *val);
-    g_free (val);
 
+    gncCustomerSetCredit (pdata->customer, dom_tree_to_gnc_numeric (node));
     return TRUE;
 }
 

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

@@ -229,13 +229,8 @@ static gboolean
 employee_workday_handler (xmlNodePtr node, gpointer employee_pdata)
 {
     struct employee_pdata* pdata = static_cast<decltype (pdata)> (employee_pdata);
-    gnc_numeric* val;
-
-    val = dom_tree_to_gnc_numeric (node);
-    g_return_val_if_fail (val, FALSE);
-    gncEmployeeSetWorkday (pdata->employee, *val);
-    g_free (val);
 
+    gncEmployeeSetWorkday (pdata->employee, dom_tree_to_gnc_numeric (node));
     return TRUE;
 }
 
@@ -243,13 +238,8 @@ static gboolean
 employee_rate_handler (xmlNodePtr node, gpointer employee_pdata)
 {
     struct employee_pdata* pdata = static_cast<decltype (pdata)> (employee_pdata);
-    gnc_numeric* val;
-
-    val = dom_tree_to_gnc_numeric (node);
-    g_return_val_if_fail (val, FALSE);
-    gncEmployeeSetRate (pdata->employee, *val);
-    g_free (val);
 
+    gncEmployeeSetRate (pdata->employee, dom_tree_to_gnc_numeric (node));
     return TRUE;
 }
 

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

@@ -250,11 +250,7 @@ static inline gboolean
 set_numeric (xmlNodePtr node, GncEntry* entry,
              void (*func) (GncEntry* entry, gnc_numeric num))
 {
-    gnc_numeric* num = dom_tree_to_gnc_numeric (node);
+    func (entry, dom_tree_to_gnc_numeric (node));
-    g_return_val_if_fail (num, FALSE);
-
-    func (entry, *num);
-    g_free (num);
     return TRUE;
 }
 

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

@@ -382,11 +382,8 @@ static gboolean
 invoice_tochargeamt_handler (xmlNodePtr node, gpointer invoice_pdata)
 {
     struct invoice_pdata* pdata = static_cast<decltype (pdata)> (invoice_pdata);
-    gnc_numeric* num = dom_tree_to_gnc_numeric (node);
-    g_return_val_if_fail (num, FALSE);
 
-    gncInvoiceSetToChargeAmount (pdata->invoice, *num);
+    gncInvoiceSetToChargeAmount (pdata->invoice, dom_tree_to_gnc_numeric (node));
-    g_free (num);
     return TRUE;
 }
 

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

@@ -130,10 +130,7 @@ price_parse_xml_sub_node (GNCPrice* p, xmlNodePtr sub_node, QofBook* book)
     }
     else if (g_strcmp0 ("price:value", (char*)sub_node->name) == 0)
     {
-        gnc_numeric* value = dom_tree_to_gnc_numeric (sub_node);
+        gnc_price_set_value (p, dom_tree_to_gnc_numeric (sub_node));
-        if (!value) return FALSE;
-        gnc_price_set_value (p, *value);
-        g_free (value);
     }
     gnc_price_commit_edit (p);
     return TRUE;

libgnucash/backend/xml/gnc-tax-table-xml-v2.cpp

@@ -183,11 +183,8 @@ static gboolean
 ttentry_amount_handler (xmlNodePtr node, gpointer ttentry_pdata)
 {
     struct ttentry_pdata* pdata = static_cast<decltype (pdata)> (ttentry_pdata);
-    gnc_numeric* num = dom_tree_to_gnc_numeric (node);
-    g_return_val_if_fail (num, FALSE);
 
-    gncTaxTableEntrySetAmount (pdata->ttentry, *num);
+    gncTaxTableEntrySetAmount (pdata->ttentry, dom_tree_to_gnc_numeric (node));
-    g_free (num);
     return TRUE;
 }
 

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

@@ -217,14 +217,8 @@ static inline gboolean
 set_spl_gnc_num (xmlNodePtr node, Split* spl,
                  void (*func) (Split* spl, gnc_numeric gn))
 {
-    gnc_numeric* num = dom_tree_to_gnc_numeric (node);
+    func (spl, dom_tree_to_gnc_numeric (node));
-    g_return_val_if_fail (num, FALSE);
+    return TRUE;
-
-    func (spl, *num);
-
-    g_free (num);
-
-    return FALSE;
 }
 
 static gboolean

libgnucash/backend/xml/io-gncxml-v1.cpp

@@ -2967,10 +2967,7 @@ price_parse_xml_sub_node (GNCPrice* p, xmlNodePtr sub_node, QofBook* book)
     }
     else if (g_strcmp0 ("price:value", (char*)sub_node->name) == 0)
     {
-        gnc_numeric* value = dom_tree_to_gnc_numeric (sub_node);
+        gnc_price_set_value (p, dom_tree_to_gnc_numeric (sub_node));
-        if (!value) return FALSE;
-        gnc_price_set_value (p, *value);
-        g_free (value);
     }
     gnc_price_commit_edit (p);
     return TRUE;

libgnucash/backend/xml/sixtp-dom-parsers.cpp

@@ -188,19 +188,7 @@ dom_tree_to_double_kvp_value (xmlNodePtr node)
 static KvpValue*
 dom_tree_to_numeric_kvp_value (xmlNodePtr node)
 {
-    gnc_numeric* danum;
+    return new KvpValue {dom_tree_to_gnc_numeric (node)};
-    KvpValue* ret = NULL;
-
-    danum = dom_tree_to_gnc_numeric (node);
-
-    if (danum)
-    {
-        ret = new KvpValue {*danum};
-    }
-
-    g_free (danum);
-
-    return ret;
 }
 
 static KvpValue*
@@ -514,19 +502,19 @@ dom_tree_to_text (xmlNodePtr tree)
     return result;
 }
 
-gnc_numeric*
+gnc_numeric
 dom_tree_to_gnc_numeric (xmlNodePtr node)
 {
     gchar* content = dom_tree_to_text (node);
     if (!content)
-        return NULL;
+        return gnc_numeric_zero ();
 
-    gnc_numeric *ret = g_new (gnc_numeric, 1);
+    gnc_numeric num;
+    if (!string_to_gnc_numeric (content, &num))
+        num = gnc_numeric_zero ();
 
-    if (!string_to_gnc_numeric (content, ret))
-	*ret = gnc_numeric_zero ();
     g_free (content);
-    return ret;
+    return num;
 }
 
 

libgnucash/backend/xml/sixtp-dom-parsers.h

@@ -42,7 +42,7 @@ Recurrence* dom_tree_to_recurrence (xmlNodePtr node);
 time64 dom_tree_to_time64 (xmlNodePtr node);
 gboolean dom_tree_valid_time64 (time64 ts, const xmlChar* name);
 GDate* dom_tree_to_gdate (xmlNodePtr node);
-gnc_numeric* dom_tree_to_gnc_numeric (xmlNodePtr node);
+gnc_numeric dom_tree_to_gnc_numeric (xmlNodePtr node);
 gchar* dom_tree_to_text (xmlNodePtr tree);
 gboolean string_to_binary (const gchar* str,  void** v, guint64* data_len);
 gboolean dom_tree_create_instance_slots (xmlNodePtr node, QofInstance* inst);

libgnucash/backend/xml/test/test-dom-converters1.cpp

@@ -163,7 +163,6 @@ static const char*
 test_gnc_nums_internal (gnc_numeric to_test)
 {
     const char* ret = NULL;
-    gnc_numeric* to_compare = NULL;
     xmlNodePtr to_gen = NULL;
 
     to_gen = gnc_numeric_to_dom_tree ("test-num", &to_test);
@@ -173,24 +172,13 @@ test_gnc_nums_internal (gnc_numeric to_test)
     }
     else
     {
-        to_compare = dom_tree_to_gnc_numeric (to_gen);
+        gnc_numeric to_compare = dom_tree_to_gnc_numeric (to_gen);
-        if (!to_compare)
+        if (!gnc_numeric_equal (to_test, to_compare))
         {
-            ret = "no gnc_numeric parsed";
+            ret = "numerics compared different";
-        }
-        else
-        {
-            if (!gnc_numeric_equal (to_test, *to_compare))
-            {
-                ret = "numerics compared different";
-            }
         }
     }
 
-    if (to_compare)
-    {
-        g_free (to_compare);
-    }
     if (to_gen)
     {
         xmlFreeNode (to_gen);

libgnucash/backend/xml/test/test-xml-transaction.cpp

@@ -70,14 +70,12 @@ find_appropriate_node (xmlNodePtr node, Split* spl)
         {
             if (g_strcmp0 ((char*)mark2->name, "split:value") == 0)
             {
-                gnc_numeric* num = dom_tree_to_gnc_numeric (mark2);
+                gnc_numeric num = dom_tree_to_gnc_numeric (mark2);
 
-                if (gnc_numeric_equal (*num, xaccSplitGetValue (spl)))
+                if (gnc_numeric_equal (num, xaccSplitGetValue (spl)))
                 {
                     amount_good = TRUE;
                 }
-
-                g_free (num);
             }
             else if (g_strcmp0 ((char*)mark2->name, "split:account") == 0)
             {
@@ -143,44 +141,40 @@ equals_node_val_vs_split_internal (xmlNodePtr node, Split* spl)
         }
         else if (g_strcmp0 ((char*)mark->name, "split:value") == 0)
         {
-            gnc_numeric* num = dom_tree_to_gnc_numeric (mark);
+            gnc_numeric num = dom_tree_to_gnc_numeric (mark);
             gnc_numeric val = xaccSplitGetValue (spl);
 
-            if (!gnc_numeric_equal (*num, val))
+            if (!gnc_numeric_equal (num, val))
             {
-                g_free (num);
                 return g_strdup_printf ("values differ: %" G_GINT64_FORMAT "/%"
                                         G_GINT64_FORMAT " v %" G_GINT64_FORMAT
                                         "/%" G_GINT64_FORMAT,
-                                        (*num).num, (*num).denom,
+                                        num.num, num.denom,
                                         val.num, val.denom);
             }
-            g_free (num);
         }
         else if (g_strcmp0 ((char*)mark->name, "split:quantity") == 0)
         {
-            gnc_numeric* num = dom_tree_to_gnc_numeric (mark);
+            gnc_numeric num = dom_tree_to_gnc_numeric (mark);
             gnc_numeric val = xaccSplitGetAmount (spl);
 
-            if (!gnc_numeric_equal (*num, val))
+            if (!gnc_numeric_equal (num, val))
             {
                 return g_strdup_printf ("quantities differ under _equal: %"
                                         G_GINT64_FORMAT "/%" G_GINT64_FORMAT
                                         " v %" G_GINT64_FORMAT "/%"
                                         G_GINT64_FORMAT,
-                                        (*num).num, (*num).denom,
+                                        num.num, num.denom,
                                         val.num, val.denom);
             }
-            if (!gnc_numeric_equal (*num, val))
+            if (!gnc_numeric_equal (num, val))
             {
-                g_free (num);
                 return g_strdup_printf ("quantities differ: %" G_GINT64_FORMAT
                                         "/%" G_GINT64_FORMAT " v %"
                                         G_GINT64_FORMAT "/%" G_GINT64_FORMAT,
-                                        (*num).num, (*num).denom,
+                                        num.num, num.denom,
                                         val.num, val.denom);
             }
-            g_free (num);
         }
         else if (g_strcmp0 ((char*)mark->name, "split:account") == 0)
         {