GNOME Bugzilla – Bug 709329
Budgets API allows setting value for period greater than num_periods
Last modified: 2018-06-29 23:19:42 UTC
In gnc-budgets.c, it is possible to set a budget value for a period that is greater than the number of periods in the budget.
This may be a feature, not a bug. It is my understanding that the code stores the budget values in a hash table (correct me if I'm wrong).
Created attachment 256338 [details] [review] Fix Bug 709329 Unit tests included.
Review of attachment 256338 [details] [review]: Thank you for your patch. I note that you added a FIXME indicating there is no error reporting if the check fails. I would like to see some error reporting added. How much depends on how this bug can be triggered. Note that I haven't studied the budgetting code. I'm working on principle here. Can a user do something to get into the situation that this test fails ? Like if setting a wrong value for an option somewhere can lead to this check failing, then the user should get a visual feedback (gnc_error_dialog or something like that). If the bug is rather academic and can be triggered only by calling the function with values that in normal use of the program don't happen, you could write a warning in gnucash.trace (using PWARN()). I'll leave it to you to investigate this a bit further :)
Created attachment 257959 [details] [review] Fix bug 709329 Geert, you are right, this is an academic bug. I couldn't find or think of any situations where it would be triggered by normal use in the UI. I've updated the code to throw a PWARN and then return. The unit test is updated to expect this warning, using g_test_expect_message.
(In reply to comment #4) > The unit test is > updated to expect this warning, using g_test_expect_message. That API is too new: We're still using 2.28 as the minimum required version of GLib. Please use the message handling API in src/test-core/unittest-support.h.
Comment on attachment 257959 [details] [review] Fix bug 709329 Change patch status as per John's comment.
I also noticed that in my first two patches I had an off by one bug in the engine code. num_periods starts from 1, but period_num starts from 0. When I do the if statement to see if the error should be thrown, I should do the following: if (period_num >= GET_PRIVATE(budget)->num_periods)
Created attachment 258058 [details] [review] Fix off-by-one error and remove dependency on g_test_expect_message The code was changed to use g_test_log_set_fatal_handler. Then it uses g_assert_cmpint (check.hits, ==, 1); to assert that the handler was actually called. Here's the source code for that specific section: + oldlogger = g_log_set_default_handler ((GLogFunc)test_null_handler, &check); + g_test_log_set_fatal_handler ((GTestLogFatalFunc)test_checked_handler, &check); + gnc_budget_set_account_period_value(budget, acc, 12, gnc_numeric_create(100,1)); + g_assert_cmpint (check.hits, ==, 1); + g_log_set_default_handler (oldlogger, NULL); As a side note, if you apply this patch and run make check, the check fails because of a failing unit test in /engine/Transaction. But if you check the history, you will see that this test (/engine/Budget/gnc_budget_set_account_period_value) passes.
Comment on attachment 258058 [details] [review] Fix off-by-one error and remove dependency on g_test_expect_message Committed are r23335. Thank you very much !
(In reply to comment #7) > I also noticed that in my first two patches I had an off by one bug in the > engine code. num_periods starts from 1, but period_num starts from 0. When I do > the if statement to see if the error should be thrown, I should do the > following: > > if (period_num >= GET_PRIVATE(budget)->num_periods) Since this is easily missed while working on the code, I have added a comment right above the test.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Reassign version to 2.4.x so that individual 2.4 versions can be retired.
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=709329. Please update any external references or bookmarks.