XERCESC-2188 - Use-after-free on external DTD scan

[boris-kolpackov]

These are the instructions for observing the bug (before this commit):

$ git clone https://github.com/apache/xerces-c.git $ cd xerces-c
$ mkdir build
$ cd build
$ cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug .. $ make -j8
$ cp ../samples/data/personal.xml .

$ cat <<EOF >personal.dtd
<?xml encoding="ISO-8859-1"?>
<!ENTITY % nonExistentEntity SYSTEM "non-existent.ent"> %nonExistentEntity;
EOF

$ gdb samples/StdInParse
(gdb) b IGXMLScanner.cpp:1544
(gdb) run <personal.xml
1544	            fReaderMgr.pushReader(reader, declDTD);
(gdb) p declDTD
$1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68
(gdb) n
1547	            dtdScanner.scanExtSubsetDecl(false, true);
(gdb) n
1548	        }
(gdb) s
...
(gdb) s                     # The Janitor is about to delete the above declDTD.
90	        delete fData;
(gdb) p fData
$1 = (xercesc_4_0::DTDEntityDecl *) 0x49ac68
(gdb) b ReaderMgr.cpp:1024
(gdb) n
...
(gdb) n                     # Now we about to dereference the deleted declDTD.
1024	    if (curEntity && !curEntity->isExternal())
(gdb) p curEntity
$2 = (const xercesc_4_0::XMLEntityDecl *) 0x49ac68
```

[boris-kolpackov]

This fix follows the same overall idea as #47 with the following key differences:

1. It addresses the lifetime issue when throwing EndOfEntityException (mentioned in a review comment to that PR).

2. It is binary backwards-compatible so can be used for a patch release.

Besides the instructions for observing the bug under the debugger (and confirming that it is no longer observed after the fix), we've also added a direct test for ReaderMgr to our package of Xerces-C++ that can be used to reproduce the issues/confirm the fix: https://github.com/build2-packaging/xerces-c/tree/3.2.5/libxerces-c/tests/reader-mgr

So the fix is reasonably well testes and we haven't observed any regressions. We've also run our CI which covers all the major platforms/compilers (but not in C++98): https://ci.stage.build2.org/@2177ad08-5621-4300-807f-8861b54c54c0

I've also reviewed this patch and it looks good to me.

Please review and/or test and let us know if there are any issues. Note that while this commit is against the master branch, it can be cleanly cherry-picked to the xerces-3.2 branch.

[theta682]

Formatting: 4 spaces indents

[theta682]

Suggested change

	delete entity;
	delete entity;

[boris-kolpackov]

Ok, thanks, we will fixup the final version of the commit.

[theta682]

Suggested change

	fCurReaderData? fCurReaderData->getEntity() : 0;
	fCurReaderData? fCurReaderData->getEntity() : 0;

[theta682]

Suggested change

	// ---------------------------------------------------------------------
	// Constructors and Destructor
	// ---------------------------------------------------------------------
	ReaderData
	( XMLReader* const reader
	, XMLEntityDecl* const entity
	, const bool adoptEntity
	);
	~ReaderData();
	// ----------------------------------------------------------------------
	// Getter methods
	// ----------------------------------------------------------------------
	XMLReader* getReader() const;
	XMLEntityDecl* getEntity() const;
	bool getEntityAdopted() const;
	XMLEntityDecl* releaseEntity();
	// ---------------------------------------------------------------------
	// Constructors and Destructor
	// ---------------------------------------------------------------------
	ReaderData
	( XMLReader* const reader
	, XMLEntityDecl* const entity
	, const bool adoptEntity
	);
	~ReaderData();
	// ----------------------------------------------------------------------
	// Getter methods
	// ----------------------------------------------------------------------
	XMLReader* getReader() const;
	XMLEntityDecl* getEntity() const;
	bool getEntityAdopted() const;
	XMLEntityDecl* releaseEntity();

[theta682]

Suggested change

	// ---------------------------------------------------------------------
	// Unimplemented constructors and operators
	// ---------------------------------------------------------------------
	ReaderData();
	ReaderData(const ReaderData&);
	ReaderData& operator=(const ReaderData&);
	// ---------------------------------------------------------------------
	// Private data members
	//
	// fReader
	// This is the pointer to the reader object that must be destroyed
	// when this object is destroyed.
	//
	// fEntity
	// fEntityAdopted
	// This is the pointer to the entity object that, if adopted, must
	// be destroyed when this object is destroyed.
	//
	// Note that we need to keep up with which of the pushed readers
	// are pushed entity values that are being spooled. This is done
	// to avoid the problem of recursive definitions.
	// ---------------------------------------------------------------------
	XMLReader* fReader;
	XMLEntityDecl* fEntity;
	bool fEntityAdopted;
	// ---------------------------------------------------------------------
	// Unimplemented constructors and operators
	// ---------------------------------------------------------------------
	ReaderData();
	ReaderData(const ReaderData&);
	ReaderData& operator=(const ReaderData&);
	// ---------------------------------------------------------------------
	// Private data members
	//
	// Reader
	// This is the pointer to the reader object that must be destroyed
	// when this object is destroyed.
	//
	// fEntity
	// fEntityAdopted
	// This is the pointer to the entity object that, if adopted, must
	// be destroyed when this object is destroyed.
	//
	// Note that we need to keep up with which of the pushed readers
	// are pushed entity values that are being spooled. This is done
	// to avoid the problem of recursive definitions.
	// ---------------------------------------------------------------------
	XMLReader* Reader;
	XMLEntityDecl* fEntity;
	bool fEntityAdopted;

[boris-kolpackov]

This PR has been merged (with whitespace issues addressed):

master: b38ab79
xerces-3.2: e002426