Crash in hbci online transfer in master due to GList of kvp_frame

[John Ralls]

On Jul 6, 2014, at 11:04 PM, Christian Stimming <christian at cstimming.de> wrote:

> Dear John,
> 
> thanks for the pointers. It seems like the import-export/aqb use case of kvp 
> was the only place in the code that used them this way, so this is probably 
> why neither you nor anyone else so far has stumbled over this problem.
> 
> I can't even easily send you a gnucash file for reproducing this problem, 
> because this very menu item can be called only if libaqbanking can already 
> verify an existing configured online banking setup for this gnucash account. 
> That means even if I send you my gnucash file, the menu items in question 
> won't be active on your machine because you don't have my libaqbanking setup.
> 
> The immediate error is that the book's kvp slot "hbci/template-list" (see 
> below for an example section of that slot from my data file) contains a value 
> that is a GList, and those list elements are again kvp slots. When those list 
> elements are asked to be converted from the kvp_value to GList elements in 
> gvalue_list_from_kvp_value(), it calls gvalue_from_kvp_value() but there the 
> "kval" argument is of type KVP_TYPE_FRAME that runs in the PWARN("Error! 
> Attempt to transfer KvpFrame!") and returns NULL, which in turn crashes in 
> kvp_frame.cpp:1677.
> 
> I think I was the only one who used this possibility to put a GList of 
> kvp_frames in the data file. At the time in 2003, I needed a GList of 
> GncABTransTempl objects (see src/import-export/aqb/gnc-ab-trans-templ.h and 
> .c) to be stored in the data file. The solution at the time was to put the 
> GncABTransTempl fields into the slots of a kvp_frame (see 
> gnc_ab_trans_templ_to_kvp and gnc_ab_trans_templ_new_from_kvp in that file), 
> and then putting those kvp_frames into a GList and this one in turn in a kvp 
> slot. Yes, I know it was against any form of data hiding, but that's what was 
> there at the time. 
> 
> However, the data fields don't have to be touched often: This list of 
> GncABTransTempl must be read once when calling the "online transfer" menu 
> item, to populate the list of templates that can be used for the online 
> transfer. (i.e. the bank details for the transfer to "Some Recipient" should 
> be auto-filled when clicking on the item in the template list.) This list is 
> written again when closing the "online transfer" window, in gnc-ab-
> transfer.c:65 and :66, if and only if the list of available templates has been 
> modified. Which is seldomly the case. Hence, no optimization whatsoever needs 
> to be in place.
> 
> How should I access a GList of a structured data type after your kvp changes, 
> when I shouldn't make this a glist of kvp_frames anymore? What would be a good 
> alternative? Thanks a lot in advance!
> 

Christian,

This is complicated by the need for compatibility with existing data files. Without that concern you could simply encode each GncABTransTempl as a GList of string GValues, but that would fail to parse an existing data frame.

One alternative is to add

/* EFFECTIVE FRIEND FUNCTION */
extern KvpFrame *qof_instance_get_slots (const QofInstance *);

to gnc-ab-trans-templ.c and not use the properties mechanism. That's the easy way, and for the short term might be the better one because it flags the interdependency and requires the least work which may be discarded in a year or two.

Another is to make a GncABTransTempl property in QofBook and make it responsible for reading and writing the kvp. That has the unfortunate effect of creating a strong interdependency between QofBook and GncABTransTempl, which we'd obviously prefer to avoid.

A third, which heads in the direction I'd like to go with KVP, is to move the creation/persistence of GncABTransTempl to the backends. That obviously means that it needs to become a child of QofInstance so that it has edit/commit/rollback, and so it would be better to wait for that interface to be reimplemented in C++ so that you don't have to do it twice on GncABTransTempl.

Regards,
John Ralls