Hi all, [this was originally written and sent Sunday, 8 May, 2016, 19:44 UTC]
I have attached a patch which I hope now really fixes
the crashes and is thread-safe by PdfMutex and PdfMutexWrapper. I have
build-tested it on my system natively (static & shared lib build).
The reference count begins at 0 for PdfOutlineItem object not fully
part of the Outlines data structure, each insertion increasing it by 1.
The mutex is one for all instances because otherwise there might be a
too high resource consumption otherwise.
Please review, test and if accepted please apply the patch and commit
separately to the public repository. Please include the info (except
about testing) from the preceding paragraph in the commit message.
Best regards, mabri
----- Original Message -----
From: zyx <z...@litepdf.cz>
To: podofo-users@lists.sourceforge.net
Sent: Wednesday, 4 May 2016, 17:43 UTC
Subject: Re: [Podofo-users] BUG: Erase method from PdfOutlineItem sometimes
segfaults
On Tue, 2016-05-03 at 10:26 +0200, zyx wrote:
> My personal opinion on the patch itself is that the pseudo-reference-
> counting added is not needed and it's not thread safe. Also, the ref-
> count should always start at 1, not at 0, but I didn't test the patch
> yet, thus my patch-reading-opinion can be wrong.
Hi,
I tested the patch from the other thread with the Yan's test doc1.pdf
and slightly modified code:
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();
}
and that is still crashing, even with the patch applied. The valgrind
claims a use-after-free without the patch and an invalid read with the
patch applied:
==8307== Invalid read of size 8
==8307== at 0x4E33C0: PoDoFo::PdfElement::GetObject() (PdfElement.h:180)
==8307== by 0x4FFB34:
PoDoFo::PdfOutlineItem::SetPrevious(PoDoFo::PdfOutlineItem*)
(PdfOutlines.cpp:192)
==8307== by 0x50002C: PoDoFo::PdfOutlineItem::Erase() (PdfOutlines.cpp:235)
==8307== by 0x4FFFDA: PoDoFo::PdfOutlineItem::Erase() (PdfOutlines.cpp:225)
==8307== by 0x4FFFDA: PoDoFo::PdfOutlineItem::Erase() (PdfOutlines.cpp:225)
==8307== by 0x4B78AA: outline_crash() (crash.cpp:21)
==8307== by 0x4B79B7: main (crash.cpp:40)
==8307== Address 0x8 is not stack'd, malloc'd or (recently) free'd
==8307==
==8307==
==8307== Process terminating with default action of signal 11 (SIGSEGV)
==8307== Access not within mapped region at address 0x8
==8307== at 0x4E33C0: PoDoFo::PdfElement::GetObject() (PdfElement.h:180)
==8307== by 0x4FFB34:
PoDoFo::PdfOutlineItem::SetPrevious(PoDoFo::PdfOutlineItem*)
(PdfOutlines.cpp:192)
==8307== by 0x50002C: PoDoFo::PdfOutlineItem::Erase() (PdfOutlines.cpp:235)
==8307== by 0x4FFFDA: PoDoFo::PdfOutlineItem::Erase() (PdfOutlines.cpp:225)
==8307== by 0x4FFFDA: PoDoFo::PdfOutlineItem::Erase() (PdfOutlines.cpp:225)
==8307== by 0x4B78AA: outline_crash() (crash.cpp:21)
==8307== by 0x4B79B7: main (crash.cpp:40)
Bye,
zyxIndex: src/doc/PdfOutlines.h
===================================================================
--- src/doc/PdfOutlines.h (revision 1721)
+++ src/doc/PdfOutlines.h (working copy)
@@ -36,6 +36,7 @@
#include "podofo/base/PdfDefines.h"
#include "PdfElement.h"
+#include "base/util/PdfMutex.h"
namespace PoDoFo {
@@ -260,7 +261,10 @@
PdfOutlineItem* m_pLast;
PdfDestination* m_pDestination;
- PdfAction* m_pAction;
+ PdfAction* m_pAction;
+ int m_nRefCount; // Erase() deletes only last result of
+ // InsertChild() remaining, otherwise only ptr changes
+ static Util::PdfMutex m_mutex; // protects the refcount in multi-threading
};
// -----------------------------------------------------
Index: src/doc/PdfOutlines.cpp
===================================================================
--- src/doc/PdfOutlines.cpp (revision 1721)
+++ src/doc/PdfOutlines.cpp (working copy)
@@ -33,6 +33,7 @@
#include "PdfOutlines.h"
+#include "base/util/PdfMutexWrapper.h"
#include "base/PdfDefinesPrivate.h"
#include "base/PdfArray.h"
@@ -44,11 +45,14 @@
namespace PoDoFo {
+Util::PdfMutex PdfOutlineItem::m_mutex; // static (one mutex for all items)
+
PdfOutlineItem::PdfOutlineItem( const PdfString & sTitle, const PdfDestination & rDest,
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_nRefCount( 0 )
{
if( pParentOutline )
this->GetObject()->GetDictionary().AddKey( "Parent", pParentOutline->GetObject()->Reference() );
@@ -61,7 +65,8 @@
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_nRefCount( 0 )
{
if( pParentOutline )
this->GetObject()->GetDictionary().AddKey( "Parent", pParentOutline->GetObject()->Reference() );
@@ -72,7 +77,8 @@
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_nRefCount( 0 )
{
PdfReference first, next;
@@ -96,11 +102,14 @@
if( m_pParentOutline )
m_pParentOutline->SetLast( this );
}
+ Util::PdfMutexWrapper wrap( m_mutex ); // this item's refcount protection
+ ++m_nRefCount;
}
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 )
+ m_pNext( NULL ), m_pFirst( NULL ), m_pLast( NULL ), m_pDestination( NULL ), m_pAction( NULL ),
+ m_nRefCount( 0 )
{
}
@@ -134,6 +143,8 @@
this->GetObject()->GetDictionary().AddKey( "First", m_pFirst->GetObject()->Reference() );
this->GetObject()->GetDictionary().AddKey( "Last", m_pLast->GetObject()->Reference() );
+ Util::PdfMutexWrapper wrap(pItem->m_mutex); // pItem's refcount protection
+ ++(pItem->m_nRefCount);
}
PdfOutlineItem* PdfOutlineItem::CreateNext ( const PdfString & sTitle, const PdfDestination & rDest )
@@ -154,6 +165,8 @@
if( m_pParentOutline && !m_pNext->Next() )
m_pParentOutline->SetLast( m_pNext );
+ Util::PdfMutexWrapper wrap(pItem->m_mutex); // pItem's refcount protection
+ ++(pItem->m_nRefCount);
return m_pNext;
}
@@ -175,6 +188,8 @@
if( m_pParentOutline && !m_pNext->Next() )
m_pParentOutline->SetLast( m_pNext );
+ Util::PdfMutexWrapper wrap(pItem->m_mutex); // pItem's refcount protection
+ ++(pItem->m_nRefCount);
return m_pNext;
}
@@ -181,13 +196,19 @@
void PdfOutlineItem::SetPrevious( PdfOutlineItem* pItem )
{
m_pPrev = pItem;
- this->GetObject()->GetDictionary().AddKey( "Prev", m_pPrev->GetObject()->Reference() );
+ if( m_pPrev )
+ this->GetObject()->GetDictionary().AddKey( "Prev", m_pPrev->GetObject()->Reference() );
+ else
+ this->GetObject()->GetDictionary().RemoveKey( "Prev" );
}
void PdfOutlineItem::SetNext( PdfOutlineItem* pItem )
{
m_pNext = pItem;
- this->GetObject()->GetDictionary().AddKey( "Next", m_pNext->GetObject()->Reference() );
+ if( m_pNext )
+ this->GetObject()->GetDictionary().AddKey( "Next", m_pNext->GetObject()->Reference() );
+ else
+ this->GetObject()->GetDictionary().RemoveKey( "Next" );
}
void PdfOutlineItem::SetLast( PdfOutlineItem* pItem )
@@ -217,20 +238,31 @@
m_pFirst->Erase();
}
- if( m_pPrev && m_pNext )
+ if( m_pPrev )
{
- m_pPrev->SetNext ( m_pNext );
+ m_pPrev->SetNext( m_pNext );
+ }
+
+ if( m_pNext )
+ {
m_pNext->SetPrevious( m_pPrev );
}
if( !m_pPrev && m_pParentOutline )
- m_pParentOutline->SetFirst( m_pNext );
+ if ( this == m_pParentOutline->First() )
+ m_pParentOutline->SetFirst( m_pNext );
if( !m_pNext && m_pParentOutline )
- m_pParentOutline->SetLast( m_pPrev );
+ if ( this == m_pParentOutline->Last() )
+ m_pParentOutline->SetLast( m_pPrev );
m_pNext = NULL;
- delete this;
+
+ Util::PdfMutexWrapper wrap( m_mutex ); // protection for the refcount
+ if( m_nRefCount > 0 )
+ --m_nRefCount;
+ if( m_nRefCount == 0 )
+ delete this;
}
void PdfOutlineItem::SetDestination( const PdfDestination & rDest )
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users
