Loading src/html.cpp +37 −22 Original line number Diff line number Diff line Loading @@ -910,23 +910,31 @@ void libhtmlpp::HtmlElement::remove(libhtmlpp::Element* el){ if(!el) return; Element *parent = el->_parentElement; Element *prevSibling = el->_prevElement; // Whoever currently owns `el` via a unique_ptr (parent->_childElement, // or a previous sibling's _nextElement) is about to be reassigned away // from `el` below, which destroys `el` immediately as part of that // assignment (reassigning a unique_ptr destroys whatever it used to // own) -- so everything needed from `el` must be captured first. // release() (not move/get()) detaches el's own next-sibling chain // without destroying anything, unlike move-assigning it while it's // still reachable through `el`. std::unique_ptr<Element> elsNext(el->_nextElement.release()); if(parent && parent->getType()==HtmlEl && static_cast<HtmlElement*>(parent)->_childElement.get()==el){ static_cast<HtmlElement*>(parent)->_childElement = std::move(el->_nextElement); if(static_cast<HtmlElement*>(parent)->_childElement) static_cast<HtmlElement*>(parent)->_childElement->_prevElement = nullptr; } else if(el->_prevElement){ Element *next = el->_nextElement.get(); el->_prevElement->_nextElement = std::move(el->_nextElement); if(next) next->_prevElement = el->_prevElement; if(elsNext) elsNext->_prevElement = nullptr; static_cast<HtmlElement*>(parent)->_childElement = std::move(elsNext); // destroys el } else if(prevSibling){ if(elsNext) elsNext->_prevElement = prevSibling; prevSibling->_nextElement = std::move(elsNext); // destroys el } // else: el has neither a parent nor a previous sibling (e.g. a // rootless standalone node) -- nothing to unlink it from. // rootless standalone node) -- nothing owns it via a reassignable // unique_ptr, so it's left unlinked-from but not destroyed. el->_prevElement=nullptr; el->_nextElement=nullptr; el->_parentElement=nullptr; // el must not be touched below this point in the two branches above -- // it no longer exists. } /** Loading Loading @@ -1411,17 +1419,24 @@ void libhtmlpp::Element::remove(libhtmlpp::Element* el){ Element *next=curel->_nextElement.get(); if(curel==el){ // `next` (captured above, before the move below) is the // element that ends up adjacent to curel->_prevElement once // curel is spliced out -- checking curel->_nextElement here // instead would always be empty (moved-from) and silently // skip fixing up its _prevElement, leaving it dangling. if(curel->_prevElement) curel->_prevElement->_nextElement=std::move(curel->_nextElement); if(next) next->_prevElement=curel->_prevElement; curel->_prevElement=nullptr; curel->_nextElement=nullptr; // Whoever owns curel via a unique_ptr (curel->_prevElement's // own _nextElement) is about to be reassigned away from curel // below, which destroys curel immediately as part of that // assignment -- so everything needed from curel (its previous // sibling, and its own next-sibling chain, detached via // release() rather than a move that's still reachable through // curel) must be captured first, and curel must not be // touched afterward. Element *prev = curel->_prevElement; std::unique_ptr<Element> curelsNext(curel->_nextElement.release()); if(next) next->_prevElement=prev; if(prev) prev->_nextElement=std::move(curelsNext); // destroys curel // else: curel has no previous sibling (e.g. `this` itself was // passed as `el`) -- the base class has no notion of a // parent's own child-pointer to repoint, so curel is left // unlinked-from but not destroyed. return; } curel=next; Loading test/htmlparenttest.cpp +9 −6 Original line number Diff line number Diff line Loading @@ -118,8 +118,11 @@ int main(){ static_cast<libhtmlpp::HtmlElement*>(&root)->getElementbyTag("ul"); check(ul != nullptr, "found <ul>"); if (ul) { libhtmlpp::Element *first = ul->firstChild(); ul->remove(first); // remove() destroys the node it's given (matching its only // real caller, insertChild, which never touches the old child // again) -- so nothing about the removed node itself is // checked below, only what remains in `ul`. ul->remove(ul->firstChild()); libhtmlpp::Element *newFirst = ul->firstChild(); check(newFirst != nullptr, "ul still has a first child after removing the old one"); Loading @@ -127,13 +130,13 @@ int main(){ libhtmlpp::HtmlElement *newFirstHel = static_cast<libhtmlpp::HtmlElement*>(newFirst); check(newFirstHel->getAtributte("id") == "b", "ul's first child is now \"b\" (the old first child's next sibling)"); check(newFirstHel->prevElement() == nullptr, "the new first child's prevElement is cleared (no dangling back-pointer)"); libhtmlpp::Element *third = newFirst->nextElement(); check(third != nullptr && static_cast<libhtmlpp::HtmlElement*>(third)->getAtributte("id") == "c", "the rest of the sibling chain (\"c\") survived (the original bug lost it)"); } check(first->parentElement() == nullptr && first->nextElement() == nullptr, "the removed node's own parent/next links are cleared"); } } Loading @@ -149,6 +152,8 @@ int main(){ check(ul != nullptr && b != nullptr, "found <ul> and \"b\""); if (ul && b) { libhtmlpp::Element *a = ul->firstChild(); // remove() destroys `b` (see the first-child case's comment) // -- `b` must not be dereferenced after this call. ul->remove(b); // previously: null-pointer dereference crash check(true, "removing a middle child (reached via sibling advancement) doesn't crash"); Loading @@ -159,8 +164,6 @@ int main(){ check(c->prevElement() == a, "\"c\"'s prevElement correctly points to \"a\", not left dangling at removed \"b\""); } check(b->parentElement() == nullptr && b->nextElement() == nullptr && b->prevElement() == nullptr, "the removed node's own parent/prev/next links are all cleared"); } } Loading Loading
src/html.cpp +37 −22 Original line number Diff line number Diff line Loading @@ -910,23 +910,31 @@ void libhtmlpp::HtmlElement::remove(libhtmlpp::Element* el){ if(!el) return; Element *parent = el->_parentElement; Element *prevSibling = el->_prevElement; // Whoever currently owns `el` via a unique_ptr (parent->_childElement, // or a previous sibling's _nextElement) is about to be reassigned away // from `el` below, which destroys `el` immediately as part of that // assignment (reassigning a unique_ptr destroys whatever it used to // own) -- so everything needed from `el` must be captured first. // release() (not move/get()) detaches el's own next-sibling chain // without destroying anything, unlike move-assigning it while it's // still reachable through `el`. std::unique_ptr<Element> elsNext(el->_nextElement.release()); if(parent && parent->getType()==HtmlEl && static_cast<HtmlElement*>(parent)->_childElement.get()==el){ static_cast<HtmlElement*>(parent)->_childElement = std::move(el->_nextElement); if(static_cast<HtmlElement*>(parent)->_childElement) static_cast<HtmlElement*>(parent)->_childElement->_prevElement = nullptr; } else if(el->_prevElement){ Element *next = el->_nextElement.get(); el->_prevElement->_nextElement = std::move(el->_nextElement); if(next) next->_prevElement = el->_prevElement; if(elsNext) elsNext->_prevElement = nullptr; static_cast<HtmlElement*>(parent)->_childElement = std::move(elsNext); // destroys el } else if(prevSibling){ if(elsNext) elsNext->_prevElement = prevSibling; prevSibling->_nextElement = std::move(elsNext); // destroys el } // else: el has neither a parent nor a previous sibling (e.g. a // rootless standalone node) -- nothing to unlink it from. // rootless standalone node) -- nothing owns it via a reassignable // unique_ptr, so it's left unlinked-from but not destroyed. el->_prevElement=nullptr; el->_nextElement=nullptr; el->_parentElement=nullptr; // el must not be touched below this point in the two branches above -- // it no longer exists. } /** Loading Loading @@ -1411,17 +1419,24 @@ void libhtmlpp::Element::remove(libhtmlpp::Element* el){ Element *next=curel->_nextElement.get(); if(curel==el){ // `next` (captured above, before the move below) is the // element that ends up adjacent to curel->_prevElement once // curel is spliced out -- checking curel->_nextElement here // instead would always be empty (moved-from) and silently // skip fixing up its _prevElement, leaving it dangling. if(curel->_prevElement) curel->_prevElement->_nextElement=std::move(curel->_nextElement); if(next) next->_prevElement=curel->_prevElement; curel->_prevElement=nullptr; curel->_nextElement=nullptr; // Whoever owns curel via a unique_ptr (curel->_prevElement's // own _nextElement) is about to be reassigned away from curel // below, which destroys curel immediately as part of that // assignment -- so everything needed from curel (its previous // sibling, and its own next-sibling chain, detached via // release() rather than a move that's still reachable through // curel) must be captured first, and curel must not be // touched afterward. Element *prev = curel->_prevElement; std::unique_ptr<Element> curelsNext(curel->_nextElement.release()); if(next) next->_prevElement=prev; if(prev) prev->_nextElement=std::move(curelsNext); // destroys curel // else: curel has no previous sibling (e.g. `this` itself was // passed as `el`) -- the base class has no notion of a // parent's own child-pointer to repoint, so curel is left // unlinked-from but not destroyed. return; } curel=next; Loading
test/htmlparenttest.cpp +9 −6 Original line number Diff line number Diff line Loading @@ -118,8 +118,11 @@ int main(){ static_cast<libhtmlpp::HtmlElement*>(&root)->getElementbyTag("ul"); check(ul != nullptr, "found <ul>"); if (ul) { libhtmlpp::Element *first = ul->firstChild(); ul->remove(first); // remove() destroys the node it's given (matching its only // real caller, insertChild, which never touches the old child // again) -- so nothing about the removed node itself is // checked below, only what remains in `ul`. ul->remove(ul->firstChild()); libhtmlpp::Element *newFirst = ul->firstChild(); check(newFirst != nullptr, "ul still has a first child after removing the old one"); Loading @@ -127,13 +130,13 @@ int main(){ libhtmlpp::HtmlElement *newFirstHel = static_cast<libhtmlpp::HtmlElement*>(newFirst); check(newFirstHel->getAtributte("id") == "b", "ul's first child is now \"b\" (the old first child's next sibling)"); check(newFirstHel->prevElement() == nullptr, "the new first child's prevElement is cleared (no dangling back-pointer)"); libhtmlpp::Element *third = newFirst->nextElement(); check(third != nullptr && static_cast<libhtmlpp::HtmlElement*>(third)->getAtributte("id") == "c", "the rest of the sibling chain (\"c\") survived (the original bug lost it)"); } check(first->parentElement() == nullptr && first->nextElement() == nullptr, "the removed node's own parent/next links are cleared"); } } Loading @@ -149,6 +152,8 @@ int main(){ check(ul != nullptr && b != nullptr, "found <ul> and \"b\""); if (ul && b) { libhtmlpp::Element *a = ul->firstChild(); // remove() destroys `b` (see the first-child case's comment) // -- `b` must not be dereferenced after this call. ul->remove(b); // previously: null-pointer dereference crash check(true, "removing a middle child (reached via sibling advancement) doesn't crash"); Loading @@ -159,8 +164,6 @@ int main(){ check(c->prevElement() == a, "\"c\"'s prevElement correctly points to \"a\", not left dangling at removed \"b\""); } check(b->parentElement() == nullptr && b->nextElement() == nullptr && b->prevElement() == nullptr, "the removed node's own parent/prev/next links are all cleared"); } } Loading