Re: [GNC-dev] [gnucash-de] Test gnucash-3.7-2019-10-13-git-3.7-131-g57e403b04+.setup.exe (Windows)

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] [gnucash-de] Test gnucash-3.7-2019-10-13-git-3.7-131-g57e403b04+.setup.exe (Windows)

Martin Preuss-3
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?


Regards
Martin
--
"Things are only impossible until they're not"
_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] [gnucash-de] Test gnucash-3.7-2019-10-13-git-3.7-131-g57e403b04+.setup.exe (Windows)

John Ralls-2


> On Oct 13, 2019, at 1:01 PM, Martin Preuss <[hidden email]> 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
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel