> On Oct 13, 2019, at 1:01 PM, Martin Preuss <mar...@aqbanking.de> wrote:
>
> Hi,
>
> as you know there are hard to debug crashes with current Gnucash GIT
> and branch-6 of AqBanking on Windows. These crashes don't seem to happen
> under Linux or in other apps.
>
> Because of that I ran gnucash through valgrind (with your
> gnucash-valgrind script).
>
> Valgrind reports multiple problems like this:
>
> --------------------------------------------------------------------------x8
>
> ==32275== 152 errors in context 65 of 444:
> ==32275== Mismatched free() / delete / delete []
> ==32275== at 0x4C30D3B: free (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==32275== by 0x893DF1C: spl_id_handler(_xmlNode*, void*)
> (gnc-transaction-xml-v2.cpp:240)
> ==32275== by 0x895D0BF: gnc_xml_set_data(char const*, _xmlNode*,
> void*, dom_tree_handler*) (sixtp-dom-parsers.cpp:809)
> ==32275== by 0x895D1B5: dom_tree_generic_parse(_xmlNode*,
> dom_tree_handler*, void*) (sixtp-dom-parsers.cpp:840)
> ==32275== by 0x893E43D: dom_tree_to_split(_xmlNode*, _QofBook*)
> (gnc-transaction-xml-v2.cpp:391)
> ==32275== by 0x893E8A9: trn_splits_handler(_xmlNode*, void*)
> (gnc-transaction-xml-v2.cpp:548)
> ==32275== by 0x895D0BF: gnc_xml_set_data(char const*, _xmlNode*,
> void*, dom_tree_handler*) (sixtp-dom-parsers.cpp:809)
> ==32275== by 0x895D1B5: dom_tree_generic_parse(_xmlNode*,
> dom_tree_handler*, void*) (sixtp-dom-parsers.cpp:840)
> ==32275== by 0x893EAA5: dom_tree_to_transaction(_xmlNode*, _QofBook*)
> (gnc-transaction-xml-v2.cpp:628)
> ==32275== by 0x893E97F: gnc_transaction_end_handler(void*, _GSList*,
> _GSList*, void*, void*, void**, char const*)
> (gnc-transaction-xml-v2.cpp:599)
> ==32275== by 0x896026D: sixtp_sax_end_handler(void*, unsigned char
> const*) (sixtp.cpp:541)
> ==32275== by 0xBE2D5D1: ??? (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE33E7A: xmlParseElement (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE3328E: xmlParseContent (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE33B72: xmlParseElement (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE3328E: xmlParseContent (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE33B72: xmlParseElement (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE3438A: xmlParseDocument (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0x8960732: sixtp_parse_file_common(sixtp*,
> _xmlParserCtxt*, void*, void*, void**) (sixtp.cpp:719)
> ==32275== by 0x89608EC: sixtp_parse_fd(sixtp*, _IO_FILE*, void*,
> void*, void**) (sixtp.cpp:794)
> ==32275== by 0x8943EAA: gnc_xml_parse_fd(sixtp*, _IO_FILE*, int
> (*)(char const*, void*, void*), void*, void*) (io-gncxml-gen.cpp:60)
> ==32275== by 0x894BABC:
> qof_session_load_from_xml_file_v2_full(GncXmlBackend*, _QofBook*, void
> (*)(_xmlParserCtxt*, void*), void*, QofBookFileType) (io-gncxml-v2.cpp:808)
> ==32275== by 0x894BD6A:
> qof_session_load_from_xml_file_v2(GncXmlBackend*, _QofBook*,
> QofBookFileType) (io-gncxml-v2.cpp:874)
> ==32275== by 0x8940512: GncXmlBackend::load(_QofBook*,
> QofBackendLoadType) (gnc-xml-backend.cpp:251)
> ==32275== by 0x5FC580F: QofSessionImpl::load(void (*)(char const*,
> double)) (qofsession.cpp:216)
> ==32275== Address 0x27adc700 is 0 bytes inside a block of size 16 alloc'd
> ==32275== at 0x4C3017F: operator new(unsigned long) (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==32275== by 0x5F63E26: guid_malloc (guid.cpp:107)
> ==32275== by 0x5F63E7B: guid_copy (guid.cpp:124)
> ==32275== by 0x5F63F58: guid_new (guid.cpp:155)
> ==32275== by 0x895B9D5: dom_tree_to_guid(_xmlNode*)
> (sixtp-dom-parsers.cpp:64)
> ==32275== by 0x893DEB8: spl_id_handler(_xmlNode*, void*)
> (gnc-transaction-xml-v2.cpp:235)
> ==32275== by 0x895D0BF: gnc_xml_set_data(char const*, _xmlNode*,
> void*, dom_tree_handler*) (sixtp-dom-parsers.cpp:809)
> ==32275== by 0x895D1B5: dom_tree_generic_parse(_xmlNode*,
> dom_tree_handler*, void*) (sixtp-dom-parsers.cpp:840)
> ==32275== by 0x893E43D: dom_tree_to_split(_xmlNode*, _QofBook*)
> (gnc-transaction-xml-v2.cpp:391)
> ==32275== by 0x893E8A9: trn_splits_handler(_xmlNode*, void*)
> (gnc-transaction-xml-v2.cpp:548)
> ==32275== by 0x895D0BF: gnc_xml_set_data(char const*, _xmlNode*,
> void*, dom_tree_handler*) (sixtp-dom-parsers.cpp:809)
> ==32275== by 0x895D1B5: dom_tree_generic_parse(_xmlNode*,
> dom_tree_handler*, void*) (sixtp-dom-parsers.cpp:840)
> ==32275== by 0x893EAA5: dom_tree_to_transaction(_xmlNode*, _QofBook*)
> (gnc-transaction-xml-v2.cpp:628)
> ==32275== by 0x893E97F: gnc_transaction_end_handler(void*, _GSList*,
> _GSList*, void*, void*, void**, char const*)
> (gnc-transaction-xml-v2.cpp:599)
> ==32275== by 0x896026D: sixtp_sax_end_handler(void*, unsigned char
> const*) (sixtp.cpp:541)
> ==32275== by 0xBE2D5D1: ??? (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE33E7A: xmlParseElement (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE3328E: xmlParseContent (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE33B72: xmlParseElement (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE3328E: xmlParseContent (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE33B72: xmlParseElement (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0xBE3438A: xmlParseDocument (in
> /usr/lib/x86_64-linux-gnu/libxml2.so.2.9.4)
> ==32275== by 0x8960732: sixtp_parse_file_common(sixtp*,
> _xmlParserCtxt*, void*, void*, void**) (sixtp.cpp:719)
> ==32275== by 0x89608EC: sixtp_parse_fd(sixtp*, _IO_FILE*, void*,
> void*, void**) (sixtp.cpp:794)
> ==32275== by 0x8943EAA: gnc_xml_parse_fd(sixtp*, _IO_FILE*, int
> (*)(char const*, void*, void*), void*, void*) (io-gncxml-gen.cpp:60)
> ==32275==
> --------------------------------------------------------------------------x8
>
> As you can see valgrind reports guid to be allocated with "operator new"
> and released with "free".
>
> First I thought that shouldn't be a problem, and at least under Linux it
> isn't. But then I found this:
>
> http://valgrind.org/docs/manual/mc-manual.html
>
> In chapter 4.2.5 it says that on some systems this mismatch could lead
> to crashes (e.g. on Solaris). Does someone here know whether this is a
> problem with Windows?
Martin,
Allocating with operator new and freeing with free instead of delete is
absolutely undefined behavior, but we don't allocate GncGUID with operator new,
we allocate them with a function guid_new()
(https://github.com/Gnucash/gnucash/blob/maint/libgnucash/engine/guid.cpp#L152).
It calls on boost::uuid::random_generator, which generates a 128-bit UUID on
the stack, and calls guid_copy
(https://github.com/Gnucash/gnucash/blob/maint/libgnucash/engine/guid.cpp#L121)
mallocs memory, and memcopies the UUID from the stack into the malloced memory.
Freeing it with free() is correct behavior.
Let's look at the first example,
https://github.com/Gnucash/gnucash/blob/maint/libgnucash/backend/xml/gnc-transaction-xml-v2.cpp#L240,
where we call g_free(tmp). tmp was initialized at line 235 with
`dom_tree_to_guid`, whose implementation is at
https://github.com/Gnucash/gnucash/blob/maint/libgnucash/backend/xml/sixtp-dom-parsers.cpp#L41
that calls 'guid_new.'
g_free (https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gmem.c#L193) is
just a wrapper for plain old free, which is the right function to release
memory allocated with malloc.
Now it's true that allocating 128 bits and passing a pointer instead of just
passing the 128 bits on the stack is dumb, but that decision was made a long
time ago and fixing stuff like that is way down the priority list. Absent a
double free or access-after-free it's not the cause of any crashes.
Do you have any stack traces of actual crashes?
Regards,
John Ralls
_______________________________________________
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
