Hi zyx, hi all,
finally I've got around to really fixing and doingsome run-testing
my patch for the memory management in PdfOutlineItem (methods
InsertChild and Erase, new copy constructor). I've tested with one
program expected to throw an exception ("crash" in the name,
originally it really crashed) and one to work (sorry, no Write()
call in it) using GCC 5.2.1 with -fsanitize=address.
The patch and the sources for the test programs are attached here.
Please get the reproducer from the original report, my mail provider
is picky about such files (I didn't get some mails from this mailing
list probably due to that, thank goodness for the archive on the Web).
What regards the security policy I'm under, also affecting un-audited,
un-signed (with some trusted key) source code, I very much hope the
attached files (contributions) can successfully restore my honour.
Best regards, mabri
> zyx <z...@litepdf.cz> has written on 24 March 2017 at 18:52:
>
> Hi,
>
> On Sun, 2017-03-12 at 00:38 +0100, Matthew Brincke wrote:
>
> > the attached patch on this e-mail should be complete now. Then I think it's
> > correct, I have tested it now (test code also attached).
>
> erm, well, it's still incomplete, still causing build break:
> podofo/trunk/src/doc/PdfOutlines.cpp:127:6: error: prototype for
> ‘void PoDoFo::PdfOutlineItem::InsertChildInternal(const
> PoDoFo::PdfOutlineItem*, bool)’ does not match any in class
> ‘PoDoFo::PdfOutlineItem’
> void PdfOutlineItem::InsertChildInternal( const PdfOutlineItem*
> pItem, bool bCheckParent )
> ^~~~~~~~~~~~~~
> In file included from
> podofo/trunk/src/doc/PdfOutlines.cpp:34:0:
> podofo/trunk/src/doc/PdfOutlines.h:223:10: error: candidate is: void
> PoDoFo::PdfOutlineItem::InsertChildInternal(PoDoFo::PdfOutlineItem*,
> bool)
> void InsertChildInternal( PdfOutlineItem* pItem, bool
> bCheckParent );
>
> And the patch is wrong anyway, the pItem cannot be 'const'. And I do
> want to insert a subtree, not only one node at a time.
>
> > just partial-linking to my PdfOutlines version, otherwise using
> > the Debian stretch libpodofo0.9.4 due to security policy
>
> Why? Why? Why? Why do you use outdated PoDoFo version in an obscure
> way? I do not understand it. You do not build into the system prefix,
> do you? I do not understand it. You even do not need to 'make install'
> to test your code.
>
> Having a svn checkout with a well-setup svn (to make 'diff' called with
> '-up' argument) would save you, and all interested, plenty of time. And
> a shame as well.
>
> > I mean that g++ 5.2 -fsanitize=address (AddressSanitizer) claimed
> > lines 82 and 90 as sources of memory leaks
>
> I believe it's because of your change, namely these lines:
> // insert only the item itself, without references to others
> pItemCopy->SetNext( NULL );
> pItemCopy->SetFirst( NULL );
>
> To sum it, your patch is not correct, I'm sorry.
>
> Bye,
>
> zyx
> --
> http://www.litePDF.cz i...@litepdf.cz
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>
> _______________________________________________
> Podofo-users mailing list
> Podofo-users@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/podofo-users
--- PdfOutlines.h (revision 1838)
+++ PdfOutlines.h (working copy)
@@ -72,6 +72,14 @@ class PODOFO_DOC_API PdfOutlineItem : pu
public:
virtual ~PdfOutlineItem();
+ /** Copy constructor. Changes the source to remove its subtree ownership,
+ which is transferred to the copy, i.e. the subtree can only be
+ Erase()d from the copy after this copy constructor has been used.
+ Calling Erase() on any subtree item affects both trees, because
+ source and copy share their subtree to conserve memory (and save time).
+ */
+ PdfOutlineItem(PdfOutlineItem& source);
+
/** Create a PdfOutlineItem that is a child of this item
* \param sTitle title of this item
* \param rDest destination of this item
@@ -95,9 +103,13 @@ class PODOFO_DOC_API PdfOutlineItem : pu
* would be broken. If this prerequisite is violated, a PdfError
* exception (code ePdfError_OutlineItemAlreadyPresent) is thrown and
* nothing is changed.
- * The item inserted is not copied, i.e. Erase() calls affect the original!
- * Therefore also shared ownership is in effect, i.e. deletion by where it
- * comes from damages the data structure it's inserted into.
+ * The item inserted is copied if it is not either a PdfOutlines instance
+ * or otherwise having not having a parent outline. The copy constructor
+ * of this class (PdfOutlineItem) is used for that, i.e. the subtree isn't
+ * copied, but shared with the item's source, this tree it's inserted into
+ * getting ownership of the subtree. The only caveat: calling Erase() on
+ * any subtree item affects both trees, yet they stay valid except as is
+ * given in the documentation of PdfOutlineItem::Erase().
*
* \param pItem an existing outline item
*/
@@ -137,6 +149,14 @@ class PODOFO_DOC_API PdfOutlineItem : pu
*/
void Erase();
+ /**
+ * \returns whether this item has ownership of its subtree, i.e. whether
+ an Erase() call on this item erases its subtree too (until an
+ * item without such owership is encountered, that item's subtree
+ stays intact, then the erasing continues on the same level).
+ */
+ inline void HasSubtreeOwnership();
+
/** Set the destination of this outline.
* \param rDest the destination
*/
@@ -269,6 +289,7 @@ class PODOFO_DOC_API PdfOutlineItem : pu
PdfDestination* m_pDestination;
PdfAction* m_pAction;
+ bool m_bPropagateErase; // subtree owership flag
};
// -----------------------------------------------------
--- PdfOutlines.cpp (revision 1838)
+++ PdfOutlines.cpp (working copy)
@@ -48,7 +48,7 @@ PdfOutlineItem::PdfOutlineItem( const Pd
PdfOutlineItem* pParentOutline, PdfVecObjects* pParent )
: PdfElement( NULL, pParent ),
m_pParentOutline( pParentOutline ), m_pPrev( NULL ), m_pNext( NULL ),
- m_pFirst( NULL ), m_pLast( NULL ), m_pDestination( NULL ), m_pAction( NULL )
+ m_pFirst( NULL ), m_pLast( NULL ), m_pDestination( NULL ), m_pAction( NULL ), m_bPropagateErase( true )
{
if( pParentOutline )
this->GetObject()->GetDictionary().AddKey( "Parent", pParentOutline->GetObject()->Reference() );
@@ -61,7 +61,7 @@ PdfOutlineItem::PdfOutlineItem( const Pd
PdfOutlineItem* pParentOutline, PdfVecObjects* pParent )
: PdfElement( NULL, pParent ),
m_pParentOutline( pParentOutline ), m_pPrev( NULL ), m_pNext( NULL ),
- m_pFirst( NULL ), m_pLast( NULL ), m_pDestination( NULL ), m_pAction( NULL )
+ m_pFirst( NULL ), m_pLast( NULL ), m_pDestination( NULL ), m_pAction( NULL ), m_bPropagateErase( true )
{
if( pParentOutline )
this->GetObject()->GetDictionary().AddKey( "Parent", pParentOutline->GetObject()->Reference() );
@@ -72,7 +72,7 @@ PdfOutlineItem::PdfOutlineItem( const Pd
PdfOutlineItem::PdfOutlineItem( PdfObject* pObject, PdfOutlineItem* pParentOutline, PdfOutlineItem* pPrevious )
: PdfElement( NULL, pObject ), m_pParentOutline( pParentOutline ), m_pPrev( pPrevious ),
- m_pNext( NULL ), m_pFirst( NULL ), m_pLast( NULL ), m_pDestination( NULL ), m_pAction( NULL )
+ m_pNext( NULL ), m_pFirst( NULL ), m_pLast( NULL ), m_pDestination( NULL ), m_pAction( NULL ), m_bPropagateErase( true )
{
PdfReference first, next;
@@ -87,7 +87,7 @@ PdfOutlineItem::PdfOutlineItem( PdfObjec
next = this->GetObject()->GetDictionary().GetKey("Next")->GetReference();
PdfObject* pObj = pObject->GetOwner()->GetObject( next );
- m_pNext = new PdfOutlineItem( pObj, NULL, this );
+ m_pNext = new PdfOutlineItem( pObj, pParentOutline, this );
}
else
{
@@ -99,15 +99,36 @@ PdfOutlineItem::PdfOutlineItem( PdfObjec
}
PdfOutlineItem::PdfOutlineItem( PdfVecObjects* pParent )
- : PdfElement( "Outlines", pParent ), m_pParentOutline( NULL ), m_pPrev( NULL ),
- m_pNext( NULL ), m_pFirst( NULL ), m_pLast( NULL ), m_pDestination( NULL ), m_pAction( NULL )
+ : PdfElement( "Outlines", pParent ), m_pParentOutline( NULL ),
+ m_pPrev( NULL ), m_pNext( NULL ), m_pFirst( NULL ), m_pLast( NULL ),
+ m_pDestination( NULL ), m_pAction( NULL ), m_bPropagateErase( true )
{
}
PdfOutlineItem::~PdfOutlineItem()
{
delete m_pNext;
- delete m_pFirst;
+ if (m_bPropagateErase)
+ delete m_pFirst;
+}
+
+// no copying for const source! (would lead to ownership confusion for subtree)
+PdfOutlineItem::PdfOutlineItem(PdfOutlineItem& source)
+ : PdfElement( source )
+{
+ if (!source.m_bPropagateErase)
+ m_bPropagateErase = false; // permissive, no throw for "non-owner copy"
+ else m_bPropagateErase = true;
+
+ source.m_bPropagateErase = false; // copy receives ownership of subtree
+
+ m_pParentOutline = source.m_pParentOutline;
+ m_pPrev = source.m_pPrev;
+ m_pNext = source.m_pNext;
+ m_pFirst = source.m_pFirst;
+ m_pLast = source.m_pLast;
+ m_pDestination = source.m_pDestination;
+ m_pAction = source.m_pAction;
}
PdfOutlineItem* PdfOutlineItem::CreateChild( const PdfString & sTitle, const PdfDestination & rDest )
@@ -162,13 +183,30 @@ void PdfOutlineItem::InsertChildInternal
PODOFO_RAISE_ERROR( ePdfError_OutlineItemAlreadyPresent );
}
+ PdfOutlineItem* pItemToInsert;
+
+ // only duplicate item if it is neither from CreateChild() nor PdfOutlines
+ if (bCheckParent && pItem->m_pParentOutline) // no parent <=> is PdfOutlines
+ {
+ pItemToInsert = new PdfOutlineItem( *pItem );
+
+ // only insert the item and its subtree (children), no same-level items
+ pItemToInsert->SetNext( NULL );
+ pItemToInsert->SetPrevious( NULL );
+ }
+ else pItemToInsert = pItem;
+
+ pItemToInsert->m_pParentOutline = this;
+ pItemToInsert->GetObject()->GetDictionary().AddKey( "Parent",
+ this->GetObject()->Reference() );
+
if( m_pLast )
{
- m_pLast->SetNext( pItem );
- pItem->SetPrevious( m_pLast );
+ m_pLast->SetNext( pItemToInsert );
+ pItemToInsert->SetPrevious( m_pLast );
}
- m_pLast = pItem;
+ m_pLast = pItemToInsert;
if( !m_pFirst )
m_pFirst = m_pLast;
@@ -257,11 +295,14 @@ void PdfOutlineItem::SetFirst( PdfOutlin
void PdfOutlineItem::Erase()
{
- while( m_pFirst )
+ if ( m_bPropagateErase )
{
- // erase will set a new first
- // if it has a next item
- m_pFirst->Erase();
+ while( m_pFirst )
+ {
+ // erase will set a new first
+ // if it has a next item
+ m_pFirst->Erase();
+ }
}
if( m_pPrev )
#include "podofo/podofo.h"
using namespace PoDoFo;
static void
outline_crash (void)
{
PdfMemDocument doc;
const PdfMemDocument indoc1("doc1.pdf");
PdfOutlines *outout = doc.GetOutlines();
outout->CreateRoot("doc1.pdf");
PdfOutlineItem *root = doc.GetOutlines()->First();
doc.InsertPages(indoc1,0,3);
root->SetDestination(PdfDestination(doc.GetPage(0)));
PdfOutlineItem *nextroot = root->Next();
if (nextroot) root->InsertChild(nextroot);
PdfOutlineItem *x = doc.GetOutlines()->First();
x->Erase();
}
int main()
{
// PdfError::EnableDebug(true); // not in 0.9.4
try {
outline_crash();
}
catch (PdfError& e) {
e.PrintErrorMsg();
}
return 0;
}
#include "podofo/podofo.h"
#include <iostream> // DEBUG
using namespace PoDoFo;
static void
outline_crash (void)
{
PdfMemDocument doc;
PdfMemDocument indoc1("doc1.pdf");
PdfOutlines *inout1 = indoc1.GetOutlines();
PdfOutlines *outout = doc.GetOutlines();
outout->CreateRoot("doc1.pdf");
PdfOutlineItem *root = doc.GetOutlines()->First();
doc.InsertPages(indoc1,0,3);
root->SetDestination(PdfDestination(doc.GetPage(0)));
PdfOutlineItem *nextroot = root->Next();
if (nextroot) nextroot->InsertChild(inout1->First());
else std::cout << "nextroot is NULL" << std::endl;
PdfOutlineItem *x = doc.GetOutlines()->First();
x->Erase();
}
int main()
{
// PdfError::EnableDebug(true); // not in 0.9.4
try {
outline_crash();
}
catch (PdfError& e) {
e.PrintErrorMsg();
}
return 0;
}
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users
