[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: addressing CVE-2018-1311/XERCESC-2188


Yes you answered my questions. Please go ahead to prepare a patch.

/ Ola

Den tors 30 jan. 2020 09:09Hugo Lefeuvre <hle@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
    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()

    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?


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].


[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

Reply to: