crashtesting ooo84576-1.odt

prevent the OOM by detecting cycles in SwList::SwList and throwing an
exception.

(1) However, that means we need to catch the exception in
    XMLTextListBlockContext::XMLTextListBlockContext
and undo some registration, otherwise we will get a use-after-free.

The need to catch it is why I'm using an UNO exception here,
it seemed like a bad idea to throw and then catch and std::foo
exception.

(2) this is still not the end of the story, a further exception
is thrown during SwDoc destruction, for which I don't have a
solution.

Change-Id: I48be3d8acbdc0f9ca948a958f1124b158ba77ac0
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/143820
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 101 insertions, 83 deletions

diff --git a/sw/source/core/doc/list.cxx b/sw/source/core/doc/list.cxx
index aa4c06f473aa..5095d4e6c9a9 100644
--- a/sw/source/core/doc/list.cxx
+++ b/sw/source/core/doc/list.cxx
@@ -33,8 +33,18 @@ SwList::SwList( OUString sListId,
 {
     // create empty list trees for the document ranges
     const SwNode* pNode = rNodes[SwNodeOffset(0)];
+    std::vector<bool> aVisited(static_cast<sal_Int32>(rNodes.Count()), false);
     do
     {
+        SwNodeOffset nIndex = pNode->GetIndex();
+        if (aVisited[static_cast<sal_Int32>(nIndex)])
+        {
+            // crashtesting ooo84576-1.odt, which manages to trigger a broken document structure
+            // in our code. This is just a workaround to prevent an infinite loop leading to OOM.
+            SAL_WARN("sw.core", "corrupt document structure, bailing out of infinite loop");
+            throw css::uno::RuntimeException("corrupt document structure, bailing out of infinite loop");
+        }
+        aVisited[static_cast<sal_Int32>(nIndex)] = true;
         SwPaM aPam( *pNode, *pNode->EndOfSectionNode() );
 
         maListTrees.emplace_back(
@@ -46,7 +56,7 @@ SwList::SwList( OUString sListId,
         pNode = pNode->EndOfSectionNode();
         if (pNode != &rNodes.GetEndOfContent())
         {
-            SwNodeOffset nIndex = pNode->GetIndex();
+            nIndex = pNode->GetIndex();
             nIndex++;
             pNode = rNodes[nIndex];
         }
diff --git a/xmloff/source/text/XMLTextListBlockContext.cxx b/xmloff/source/text/XMLTextListBlockContext.cxx
index 7c688f4c5e6c..c22eb2e221f2 100644
--- a/xmloff/source/text/XMLTextListBlockContext.cxx
+++ b/xmloff/source/text/XMLTextListBlockContext.cxx
@@ -110,113 +110,121 @@ XMLTextListBlockContext::XMLTextListBlockContext(
 
     // Remember this list block.
     mrTxtImport.GetTextListHelper().PushListContext( this );
+    try
+    {
+        mxNumRules = XMLTextListsHelper::MakeNumRule(GetImport(), mxNumRules,
+            sParentListStyleName, msListStyleName,
+            mnLevel, &mbRestartNumbering, &mbSetDefaults );
+        if( !mxNumRules.is() )
+            return;
 
-    mxNumRules = XMLTextListsHelper::MakeNumRule(GetImport(), mxNumRules,
-        sParentListStyleName, msListStyleName,
-        mnLevel, &mbRestartNumbering, &mbSetDefaults );
-    if( !mxNumRules.is() )
-        return;
-
-    if ( mnLevel != 0 ) // root <list> element
-        return;
+        if ( mnLevel != 0 ) // root <list> element
+            return;
 
-    XMLTextListsHelper& rTextListsHelper( mrTxtImport.GetTextListHelper() );
-    // Inconsistent behavior regarding lists (#i92811#)
-    OUString sListStyleDefaultListId;
-    {
-        uno::Reference< beans::XPropertySet > xNumRuleProps( mxNumRules, UNO_QUERY );
-        if ( xNumRuleProps.is() )
+        XMLTextListsHelper& rTextListsHelper( mrTxtImport.GetTextListHelper() );
+        // Inconsistent behavior regarding lists (#i92811#)
+        OUString sListStyleDefaultListId;
         {
-            uno::Reference< beans::XPropertySetInfo > xNumRulePropSetInfo(
-                                        xNumRuleProps->getPropertySetInfo());
-            if (xNumRulePropSetInfo.is() &&
-                xNumRulePropSetInfo->hasPropertyByName(
-                     s_PropNameDefaultListId))
+            uno::Reference< beans::XPropertySet > xNumRuleProps( mxNumRules, UNO_QUERY );
+            if ( xNumRuleProps.is() )
             {
-                xNumRuleProps->getPropertyValue(s_PropNameDefaultListId)
-                    >>= sListStyleDefaultListId;
-                SAL_WARN_IF( sListStyleDefaultListId.isEmpty(), "xmloff",
-                            "no default list id found at numbering rules instance. Serious defect." );
+                uno::Reference< beans::XPropertySetInfo > xNumRulePropSetInfo(
+                                            xNumRuleProps->getPropertySetInfo());
+                if (xNumRulePropSetInfo.is() &&
+                    xNumRulePropSetInfo->hasPropertyByName(
+                         s_PropNameDefaultListId))
+                {
+                    xNumRuleProps->getPropertyValue(s_PropNameDefaultListId)
+                        >>= sListStyleDefaultListId;
+                    SAL_WARN_IF( sListStyleDefaultListId.isEmpty(), "xmloff",
+                                "no default list id found at numbering rules instance. Serious defect." );
+                }
             }
         }
-    }
-    if ( msListId.isEmpty() )  // no text:id property found
-    {
-        sal_Int32 nUPD( 0 );
-        sal_Int32 nBuild( 0 );
-        const bool bBuildIdFound = GetImport().getBuildIds( nUPD, nBuild );
-        if ( rImport.IsTextDocInOOoFileFormat() ||
-             ( bBuildIdFound && nUPD == 680 ) )
+        if ( msListId.isEmpty() )  // no text:id property found
         {
-            /* handling former documents written by OpenOffice.org:
-               use default list id of numbering rules instance, if existing
-               (#i92811#)
-            */
-            if ( !sListStyleDefaultListId.isEmpty() )
+            sal_Int32 nUPD( 0 );
+            sal_Int32 nBuild( 0 );
+            const bool bBuildIdFound = GetImport().getBuildIds( nUPD, nBuild );
+            if ( rImport.IsTextDocInOOoFileFormat() ||
+                 ( bBuildIdFound && nUPD == 680 ) )
             {
-                msListId = sListStyleDefaultListId;
-                if ( !bIsContinueNumberingAttributePresent &&
-                     !mbRestartNumbering &&
-                     rTextListsHelper.IsListProcessed( msListId ) )
+                /* handling former documents written by OpenOffice.org:
+                   use default list id of numbering rules instance, if existing
+                   (#i92811#)
+                */
+                if ( !sListStyleDefaultListId.isEmpty() )
                 {
-                    mbRestartNumbering = true;
+                    msListId = sListStyleDefaultListId;
+                    if ( !bIsContinueNumberingAttributePresent &&
+                         !mbRestartNumbering &&
+                         rTextListsHelper.IsListProcessed( msListId ) )
+                    {
+                        mbRestartNumbering = true;
+                    }
                 }
             }
+            if ( msListId.isEmpty() )
+            {
+                // generate a new list id for the list
+                msListId = rTextListsHelper.GenerateNewListId();
+            }
         }
-        if ( msListId.isEmpty() )
-        {
-            // generate a new list id for the list
-            msListId = rTextListsHelper.GenerateNewListId();
-        }
-    }
 
-    if ( bIsContinueNumberingAttributePresent && !mbRestartNumbering &&
-         msContinueListId.isEmpty() )
-    {
-        const OUString& Last( rTextListsHelper.GetLastProcessedListId() );
-        if ( rTextListsHelper.GetListStyleOfLastProcessedList() == msListStyleName
-             && Last != msListId )
+        if ( bIsContinueNumberingAttributePresent && !mbRestartNumbering &&
+             msContinueListId.isEmpty() )
         {
-            msContinueListId = Last;
+            const OUString& Last( rTextListsHelper.GetLastProcessedListId() );
+            if ( rTextListsHelper.GetListStyleOfLastProcessedList() == msListStyleName
+                 && Last != msListId )
+            {
+                msContinueListId = Last;
+            }
         }
-    }
 
-    bool bContinueNumbering = bIsContinueNumberingAttributePresent && !mbRestartNumbering;
-    if (msContinueListId.isEmpty() && bContinueNumbering && GetImport().IsMSO())
-    {
-        // No "continue list" id, but continue numbering was requested. Connect to the last list of
-        // the same list style in the Word case, even if there was a different list in the meantime.
-        msContinueListId = rTextListsHelper.GetLastIdOfStyleName(msListStyleName);
-    }
-
-    if ( !msContinueListId.isEmpty() )
-    {
-        if ( !rTextListsHelper.IsListProcessed( msContinueListId ) )
+        bool bContinueNumbering = bIsContinueNumberingAttributePresent && !mbRestartNumbering;
+        if (msContinueListId.isEmpty() && bContinueNumbering && GetImport().IsMSO())
         {
-            msContinueListId.clear();
+            // No "continue list" id, but continue numbering was requested. Connect to the last list of
+            // the same list style in the Word case, even if there was a different list in the meantime.
+            msContinueListId = rTextListsHelper.GetLastIdOfStyleName(msListStyleName);
         }
-        else
+
+        if ( !msContinueListId.isEmpty() )
         {
-            // search continue list chain for master list and
-            // continue the master list.
-            OUString sTmpStr =
-                rTextListsHelper.GetContinueListIdOfProcessedList( msContinueListId );
-            while ( !sTmpStr.isEmpty() )
+            if ( !rTextListsHelper.IsListProcessed( msContinueListId ) )
             {
-                msContinueListId = sTmpStr;
-
-                sTmpStr =
+                msContinueListId.clear();
+            }
+            else
+            {
+                // search continue list chain for master list and
+                // continue the master list.
+                OUString sTmpStr =
                     rTextListsHelper.GetContinueListIdOfProcessedList( msContinueListId );
+                while ( !sTmpStr.isEmpty() )
+                {
+                    msContinueListId = sTmpStr;
+
+                    sTmpStr =
+                        rTextListsHelper.GetContinueListIdOfProcessedList( msContinueListId );
+                }
             }
         }
-    }
 
-    if ( !rTextListsHelper.IsListProcessed( msListId ) )
+        if ( !rTextListsHelper.IsListProcessed( msListId ) )
+        {
+            // Inconsistent behavior regarding lists (#i92811#)
+            rTextListsHelper.KeepListAsProcessed(
+                msListId, msListStyleName, msContinueListId,
+                sListStyleDefaultListId );
+        }
+    }
+    catch (uno::Exception&)
     {
-        // Inconsistent behavior regarding lists (#i92811#)
-        rTextListsHelper.KeepListAsProcessed(
-            msListId, msListStyleName, msContinueListId,
-            sListStyleDefaultListId );
+        // pop ourselves if anything goes wrong to avoid use-after-free
+        rTxtImp.GetTextListHelper().PopListContext();
+        throw;
     }
 }