On Oct 17, 2011, at 11:34 AM, Christian Stimming wrote: > Am Montag, 17. Oktober 2011 schrieb John Ralls: >> On Sep 26, 2011, at 11:35 AM, Christian Stimming wrote: >>> Am Montag, 26. September 2011 schrieb John Ralls: >>>> I have a plan to rework the engine (the module which does all of >>>> the accounting and business calculations) in stages. Stage one is to >>>> write comprehensive tests for each class. Stage two (which will >>>> interleave with stage one) is to overhaul each class to be a >>>> well-behaved GObject class with all access to data (including KVP) via >>>> function calls instead of passing out pointers for other objects to >>>> modify at will. >>> >>> Out of curiosity: Which examples for "passing out pointers" do you have >>> in mind here? Surely there is plenty of access to KVP data by simply >>> accessing the KVP directly, which is bad, but is there also direct >>> pointer manipulation in that many places? >> >> Here's an example I found today, while investigating bug 661903: >> xaccTransGetSplitList() returns a GList* of the splits. The requestor can >> manipulate or delete the splits, unbalancing or even eviscerating the >> transaction without the transaction knowing. It's called in 23 places in C >> and 19 in Scheme. >> >> It doesn't help that Accounts also keep a list of splits and since we're >> not using GObject reference counting, there's no way to protect against >> one or both from having invalid pointers. > > Absolutely. All of the API functions passing out a GList* are prone to this > error. However, that's an inherent issue of any C API, because the only > alternative is to pass out a newly allocated GList* each time. You can easily > guess this will lead to plenty of memory leaks... well... thinking about it > again, this might not be as bad as it sounds. Maybe those functions should > indeed pass out a newly allocated GList instead of the internal one, i.e. > just > using g_list_copy. Then again, the data pointers will still be plain > non-const > pointers as there isn't a const variant of GList*...
That's only part of the problem. The bug (and it's 661093, not 661903, sorry) is reporting a crash caused by trying to access a split that's been freed. We have several objects holding references to splits (transactions, accounts, lots, and business entities come immediately to mind); in addition, register windows need to have active access to split lists in order to edit them. Aside from memory management, problems arise in notifying all of the holders of references to a split that the split isn't valid. I think that's supposed to be done with QofSignals and handlers, but there's at least one demonstrated hole in the process. There are a couple of approaches to handling all of the references, but I don't yet know which one will best enforce database integrity. They all involve getting everything into a clean GObject first so that reference counting and weak references can be used to manage object lifetime, so we can defer worrying about it for a while. Regards, John Ralls _______________________________________________ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel
