Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix "invalid read" errors from valgrind, prevent crash in XML parsing #34

Merged
merged 3 commits into from
Mar 27, 2019

Conversation

andresailer
Copy link
Contributor

BEGINRELEASENOTES

  • XMLParser: Fix "random" crash during XML parsing. Depends on XML Steering file and memory runtime behaviour, when some freed memory is overwritten
     /Marlin/source/tinyxml/src/tinyxml.cc:377: const TiXmlNode* TiXmlNode::IterateChildren(const char*, const TiXmlNode*) const: Assertion `previous->parent == this' failed.
    
  • XMLParser: Fix "invalid read errors" reported by valgrind. Objects were removed before they were used again, which probably lead to the above mentioned crashs

ENDRELEASENOTES

crash

The crash can (maybe?) be reproduced with attached steering file (renamed to xml)
break.txt

source /cvmfs/clicdp.cern.ch/iLCSoft/builds/2019-02-20/x86_64-slc6-gcc62-opt/init_ilcsoft.sh
wget https://github.com/iLCSoft/Marlin/files/3013507/break.txt -O break.xml
Marlin break.xml

invalid read

Example valgrind output

==21315== Invalid read of size 8
==21315==    at 0x4ECA406: TiXmlNode::NextSibling(char const*) const (tinyxml.cc:386)
==21315==    by 0x4C8525B: IterateChildren (tinyxml.h:570)
==21315==    by 0x4C8525B: marlin::XMLParser::parse() (XMLParser.cc:160)
==21315==    by 0x40E5AE: main (Marlin.cc:277)
==21315==  Address 0x145894c0 is 96 bytes inside a block of size 216 free'd
==21315==    at 0x4A09186: operator delete(void*) (vg_replace_malloc.c:575)
==21315==    by 0x4ECA2F3: TiXmlNode::RemoveChild(TiXmlNode*) (tinyxml.cc:327)
==21315==    by 0x4C85417: marlin::XMLParser::parse() (XMLParser.cc:179)
==21315==    by 0x40E5AE: main (Marlin.cc:277)
==21315==  Block was alloc'd at
==21315==    at 0x4A080BC: operator new(unsigned long) (vg_replace_malloc.c:333)
==21315==    by 0x4ECF3A9: TiXmlNode::Identify(char const*, TiXmlEncoding) (tinyxmlparser.cc:892)
==21315==    by 0x4ED0C45: TiXmlElement::ReadValue(char const*, TiXmlParsingData*, TiXmlEncoding) (tinyxmlparser.cc:1229)
==21315==    by 0x4ED0FDE: TiXmlElement::Parse(char const*, TiXmlParsingData*, TiXmlEncoding) (tinyxmlparser.cc:1124)
==21315==    by 0x4ECF50C: TiXmlDocument::Parse(char const*, TiXmlParsingData*, TiXmlEncoding) (tinyxmlparser.cc:767)
==21315==    by 0x4ECAE0D: TiXmlDocument::LoadFile(_IO_FILE*, TiXmlEncoding) (tinyxml.cc:1077)
==21315==    by 0x4ECB0A6: TiXmlDocument::LoadFile(char const*, TiXmlEncoding) (tinyxml.cc:953)
==21315==    by 0x4C84002: LoadFile (tinyxml.h:1409)
==21315==    by 0x4C84002: marlin::XMLParser::parse() (XMLParser.cc:28)
==21315==    by 0x40E5AE: main (Marlin.cc:277)

…nditions call

child in processConditions might be removed , but the iterateChildren was still using it

Store the next iteration already before calling processconditions
@gaede gaede merged commit a1e6a70 into iLCSoft:master Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants