Hi Yes you answered my questions. Please go ahead to prepare a patch.
/ Ola Den tors 30 jan. 2020 09:09Hugo Lefeuvre <h...@owl.eu.com> skrev: > Hi Ola, > > > > A DTDEntityDecl object is allocated and pushed into the ReaderMgr > stack. > > > ReaderMgr does not own the stack's content, so objects neither get > freed on > > > ReaderMgr::popReader(), nor on ReaderMgr::~ReaderMgr(). > > > > And it should not be freed by the code popping the object? > > I don't think so. In fact, the code popping the object is the ReaderMgr > itself: popReader is a private method called by a higher level scanning > API, e.g. ReaderMgr::getNextChar. > > $ ag popReader > src/xercesc/internal/ReaderMgr.cpp > 107: if (!popReader()) > 129: if (!popReader()) > 149: if (!popReader()) > 166: if (!popReader()) > 191: if (!popReader()) > 214: if (!popReader()) > 237: if (!popReader()) > 256: if (!popReader()) > 273: if (!popReader()) > 1056:bool ReaderMgr::popReader() > > src/xercesc/internal/ReaderMgr.hpp > 203: bool popReader(); > > > > A janitor is set to avoid leaking memory via the allocated > DTDEntityDecl. > > > Unfortunately the object is freed when the janitor gets out of scope, > at > > > which point a pointer to this object is still present within the stack. > > > > Do you mean that the janitor should not free it at this point? > > Exactly. This is too early! > > > <snip> > > # I suggest the following fix: > > > > > > Add a `bool adopt` parameter to ReaderMgr::pushReader() (default value > of > > > false), and a `RefStackOf<XMLEntityDecl>* fAdoptedStack` private > element to > > > the ReaderMgr class. > > > > > > Whenever ReaderMgr::pushReader() is called with `adopt` set to true, > also > > > push passed object onto `fAdoptedStack`. > > > > > > Whenever ReaderMgr::popReader() is called, check whether the object > being > > > removed is on top of `fAdoptedStack`. If so, remove and delete it. > > > > > > On ReaderMgr::~ReaderMgr(), delete `fAdoptedStack` and all possibly > > > remaining elements. > > > > > > > I assume that you also remove the janitor in this case? > > Yes! > > Thanks for having a look. Did I manage to answer your questions? If so, I'd > go on, implement the patch and try to get some review from upstream. > > BTW, I don't think previous messages reached the c-dev mailing list. I > tried to subscribe, let's see if this one does. > > FTR: initial message here[0]. > > cheers, > Hugo > > [0] https://lists.debian.org/debian-lts/2020/01/msg00055.html > > -- > Hugo Lefeuvre (hle) | www.owl.eu.com > RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD > ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C >
