[GNC-dev] [gnucash-de] Test gnucash-3.7-2019-10-13-git-3.7-131-g57e403b04+.setup.exe (Windows)
John Ralls
jralls at ceridwen.us
Mon Oct 14 11:06:28 EDT 2019
> On Oct 13, 2019, at 1:01 PM, Martin Preuss <martin at 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
More information about the gnucash-devel
mailing list