Improve gnc_numeric_boxed_copy_func based on discussion in PR#145

In essence
- guard against nullptr dereferencing
- free returned values

gnucash/register/ledger-core/split-register-model.c

@@ -2209,7 +2209,7 @@ gnc_template_register_get_debcred_entry (VirtualLocation virt_loc,
 {
     SplitRegister *reg = user_data;
     Split *split;
-    gnc_numeric *amount;
+    gnc_numeric *amount, amount2;
     const char * cell_name;
 
     split = gnc_split_register_get_split (reg, virt_loc.vcell_loc);
@@ -2228,11 +2228,18 @@ gnc_template_register_get_debcred_entry (VirtualLocation virt_loc,
         qof_instance_get (QOF_INSTANCE (split),
                           "sx-credit-numeric", &amount,
                           NULL);
-    if (gnc_numeric_zero_p (*amount))
+    if (!amount)
         return "";
 
-    *amount = gnc_numeric_abs (*amount);
-    return xaccPrintAmount (*amount, gnc_default_print_info (FALSE));
+    if (gnc_numeric_zero_p (*amount))
+    {
+        g_free (amount);
+        return "";
+    }
+
+    amount2 = gnc_numeric_abs (*amount);
+    g_free (amount);
+    return xaccPrintAmount (amount2, gnc_default_print_info (FALSE));
 }
 
 static void

libgnucash/app-utils/gnc-sx-instance-model.c

@@ -78,11 +78,12 @@ scrub_sx_split_numeric (Split* split, const char *debcred)
     const char *numeric = is_credit ?
         "sx-credit-numeric" : "sx-debit-numeric";
     char *formval;
-    gnc_numeric *numval;
+    gnc_numeric *numval = NULL;
     GHashTable *parser_vars = g_hash_table_new (g_str_hash, g_str_equal);
     char *error_loc;
     gnc_numeric amount = gnc_numeric_zero ();
     gboolean parse_result = FALSE;
+    gboolean num_val_changed = FALSE;
     qof_instance_get (QOF_INSTANCE (split),
 		  formula, &formval,
 		  numeric, &numval,
@@ -93,12 +94,15 @@ scrub_sx_split_numeric (Split* split, const char *debcred)
     if (!parse_result || g_hash_table_size (parser_vars) != 0)
         amount = gnc_numeric_zero ();
     g_hash_table_unref (parser_vars);
-    if (gnc_numeric_eq (amount, *numval))
-        return FALSE;
-    qof_instance_set (QOF_INSTANCE (split),
-		  numeric, &amount,
-		  NULL);
-    return TRUE;
+    if (!numval || !gnc_numeric_eq (amount, *numval))
+    {
+        qof_instance_set (QOF_INSTANCE (split),
+                          numeric, &amount,
+                          NULL);
+        num_val_changed = TRUE;
+    }
+    g_free (numval);
+    return num_val_changed;
 }
 
 /* Fixes error in pre-2.6.16 where the numeric slot wouldn't get changed if the

libgnucash/engine/gnc-numeric.cpp

@@ -1222,12 +1222,15 @@ string_to_gnc_numeric(const gchar* str, gnc_numeric *n)
  *  GValue handling
  ********************************************************************/
 static gpointer
-gnc_numeric_boxed_copy_func( gpointer in_gnc_numeric )
+gnc_numeric_boxed_copy_func( gpointer in_ptr )
 {
-    gnc_numeric* newvalue;
+    auto in_gnc_numeric = static_cast<gnc_numeric*>(in_ptr);
+    if (!in_gnc_numeric)
+        return nullptr;
 
-    newvalue = static_cast<gnc_numeric*>(g_malloc (sizeof (gnc_numeric)));
-    memcpy( newvalue, in_gnc_numeric, sizeof( gnc_numeric ) );
+    /* newvalue will be passed to g_free so we must allocate with g_malloc. */
+    auto newvalue = static_cast<gnc_numeric*>(g_malloc (sizeof (gnc_numeric)));
+    *newvalue = *in_gnc_numeric;
 
     return newvalue;
 }

libgnucash/engine/test/test-account-object.cpp

@@ -60,6 +60,8 @@ run_test (void)
     do_test (gnc_numeric_zero_p(*end), "end balance is zero");
     do_test (gnc_numeric_zero_p(delta), "delta is zero");
     do_test (gnc_numeric_zero_p(end2), "end2 balance is zero");
+    g_free (start);
+    g_free (end);
 
     /*****/
 
@@ -75,6 +77,8 @@ run_test (void)
     do_test (gnc_numeric_zero_p(delta), "end balance matches");
     delta = gnc_numeric_sub(end2, five, GNC_DENOM_AUTO, GNC_HOW_DENOM_FIXED);
     do_test (gnc_numeric_zero_p(delta), "end2 balance matches");
+    g_free (start);
+    g_free (end);
 
     /*****/
 

libgnucash/engine/test/utest-Account.cpp

@@ -625,6 +625,12 @@ test_gnc_account_create_and_destroy (void)
     g_free (notes);
     g_free (tax_code);
     g_free (tax_src);
+    g_free (start_bal);
+    g_free (start_clr_bal);
+    g_free (start_rec_bal);
+    g_free (end_bal);
+    g_free (end_clr_bal);
+    g_free (end_rec_bal);
 
     g_object_unref (acc);
     /* There's no good way to test that the object has been properly

libgnucash/engine/test/utest-Budget.c

@@ -604,6 +604,12 @@ test_gnc_account_create_and_destroy (void)
     g_free (notes);
     g_free (tax_code);
     g_free (tax_src);
+    g_free (start_bal);
+    g_free (start_clr_bal);
+    g_free (start_rec_bal);
+    g_free (end_bal);
+    g_free (end_clr_bal);
+    g_free (end_rec_bal);
 
     g_object_unref (acc);
     /* There's no good way to test that the object has been properly

libgnucash/engine/test/utest-Split.cpp

@@ -249,6 +249,8 @@ test_gnc_split_set_get_property ()
      * few leaks to save trouble; it will all work fine once the
      * refactoring is taken care of.
      */
+    g_free (r_value);
+    g_free (r_amount);
     g_object_unref (txn);
     g_object_unref (acc);
     g_object_unref (lot);