On Tue, 2017-04-25 at 00:47 +0200, Matthias Brinke wrote:
> The patch and the sources for the test programs are attached here.

        Hi,
thanks for updated patch. It still isn't perfect, you left there unused
code in .h file, but okay. I wasn't able to reproduce the crash before
the patch, neither when it had been applied, thus it truly doesn't make
things worse with the changes I tried, but the comments you wrote there
somewhat scare me. The part about "erase erases in both trees" is not
the right thing.

Imagine you want to copy subtree from another document and then that
other document free, while still having the destination document there
and making changes in it. This way you get lost the subtree which is
copied, no? I forgot what we settled with, it's a long time ago. Can
there be different use-cases? Maybe not. If you want to copy part of
the outlines from one document to another, then it should be truly
copied, complete subtree. If you want to move subtree within the tree,
then it's also clear what to do. That way Erase() is deterministic,
erase the node and its subtree.

It also didn't fix a warning reported by clang's static analyzer, see
below.

Feel free to correct me. As I said, this issue had been opened quite
long ago.
        Bye,
        zyx

podofo-0.9.5/src/doc/PdfOutlines.cpp:304:13: warning: Use of memory after it is 
freed
#            m_pFirst->Erase();
#            ^~~~~~~~
podofo-0.9.5/src/doc/PdfOutlines.cpp:298:10: note: Assuming the condition is 
true
#    if ( m_bPropagateErase )
#         ^~~~~~~~~~~~~~~~~
podofo-0.9.5/src/doc/PdfOutlines.cpp:298:5: note: Taking true branch
#    if ( m_bPropagateErase )
#    ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:300:9: note: Loop condition is true.  
Entering loop body
#        while( m_pFirst )
#        ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:304:13: note: Calling 
'PdfOutlineItem::Erase'
#            m_pFirst->Erase();
#            ^~~~~~~~~~~~~~~~~
podofo-0.9.5/src/doc/PdfOutlines.cpp:298:10: note: Assuming the condition is 
false
#    if ( m_bPropagateErase )
#         ^~~~~~~~~~~~~~~~~
podofo-0.9.5/src/doc/PdfOutlines.cpp:298:5: note: Taking false branch
#    if ( m_bPropagateErase )
#    ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:308:9: note: Assuming the condition is 
false
#    if( m_pPrev ) 
#        ^~~~~~~
podofo-0.9.5/src/doc/PdfOutlines.cpp:308:5: note: Taking false branch
#    if( m_pPrev ) 
#    ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:313:9: note: Assuming the condition is 
false
#    if( m_pNext ) 
#        ^~~~~~~
podofo-0.9.5/src/doc/PdfOutlines.cpp:313:5: note: Taking false branch
#    if( m_pNext ) 
#    ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:318:9: note: Left side of '&&' is true
#    if( !m_pPrev && m_pParentOutline && this == m_pParentOutline->First() )
#        ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:318:21: note: Assuming the condition is 
false
#    if( !m_pPrev && m_pParentOutline && this == m_pParentOutline->First() )
#                    ^~~~~~~~~~~~~~~~
podofo-0.9.5/src/doc/PdfOutlines.cpp:318:38: note: Left side of '&&' is false
#    if( !m_pPrev && m_pParentOutline && this == m_pParentOutline->First() )
#                                     ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:321:9: note: Left side of '&&' is true
#    if( !m_pNext && m_pParentOutline && this == m_pParentOutline->Last() )
#        ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:321:38: note: Left side of '&&' is false
#    if( !m_pNext && m_pParentOutline && this == m_pParentOutline->Last() )
#                                     ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:325:5: note: Memory is released
#    delete this;
#    ^~~~~~~~~~~
podofo-0.9.5/src/doc/PdfOutlines.cpp:304:13: note: Returning; memory was 
released
#            m_pFirst->Erase();
#            ^~~~~~~~~~~~~~~~~
podofo-0.9.5/src/doc/PdfOutlines.cpp:300:9: note: Loop condition is true.  
Entering loop body
#        while( m_pFirst )
#        ^
podofo-0.9.5/src/doc/PdfOutlines.cpp:304:13: note: Use of memory after it is 
freed
#            m_pFirst->Erase();
#            ^~~~~~~~
#  302|               // erase will set a new first
#  303|               // if it has a next item
#  304|->             m_pFirst->Erase();
#  305|           }
#  306|       }

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

Reply via email to