ofz#18554 sw: fix Null-dereference due to overlapping fieldmarks

The problem is that the WW8 import wants to set a fieldmark on a range
that contains only the CH_TXT_ATR_FIELDEND of another fieldmark:

(rr) p io_pDoc->GetNodes()[12]->m_Text.copy(33,10)
$30 = "\bÿÿÿ\001ÿÿÿ\001 "

MarkManager::makeMark() must check that a new fieldmark never overlaps
existing fieldmarks or meta-fields.

While at it, it looks like the test in
DocumentContentOperationsManager::DelFullPara() can't necessarily use
the passed rPam, because it obviously deletes entire nodes, but at
least SwRangeRedline::DelCopyOfSection() doesn't even set nContent
on rPam.

Also, the check in makeMark() triggers an assert in
CppunitTest_sw_uiwriter testTextFormFieldInsertion because
SwHistoryTextFieldmark::SetInDoc() was neglecting to subtract 1
from the end position for the CH_TXT_ATR_FIELDEND.

Change-Id: I46c1955dd8dd422a41dcbb9bc68dbe09075b4922
Reviewed-on: https://gerrit.libreoffice.org/83000
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
Tested-by: Caolán McNamara <caolanm@redhat.com>

5 files changed, 40 insertions, 13 deletions

diff --git a/sw/qa/core/data/ww8/pass/ofz18554-1.doc b/sw/qa/core/data/ww8/pass/ofz18554-1.doc
new file mode 100644
index 000000000000..0a6b81d78b43
--- /dev/null
+++ b/sw/qa/core/data/ww8/pass/ofz18554-1.doc
diff --git a/sw/source/core/doc/DocumentContentOperationsManager.cxx b/sw/source/core/doc/DocumentContentOperationsManager.cxx
index 995e3dcfa482..3b9433619ae6 100644
--- a/sw/source/core/doc/DocumentContentOperationsManager.cxx
+++ b/sw/source/core/doc/DocumentContentOperationsManager.cxx
@@ -60,6 +60,7 @@
 #include <UndoAttribute.hxx>
 #include <rolbck.hxx>
 #include <acorrect.hxx>
+#include <bookmrk.hxx>
 #include <ftnidx.hxx>
 #include <txtftn.hxx>
 #include <hints.hxx>
@@ -1796,6 +1797,16 @@ namespace //local functions originally from docfmt.cxx
 namespace sw
 {
 
+namespace mark
+{
+    bool IsFieldmarkOverlap(SwPaM const& rPaM)
+    {
+        std::vector<std::pair<sal_uLong, sal_Int32>> Breaks;
+        lcl_CalcBreaks(Breaks, rPaM);
+        return !Breaks.empty();
+    }
+}
+
 DocumentContentOperationsManager::DocumentContentOperationsManager( SwDoc& i_rSwdoc ) : m_rDoc( i_rSwdoc )
 {
 }
@@ -1947,9 +1958,16 @@ bool DocumentContentOperationsManager::DelFullPara( SwPaM& rPam )
     }
 
     {
-        std::vector<std::pair<sal_uLong, sal_Int32>> Breaks;
-        lcl_CalcBreaks(Breaks, rPam);
-        if (!Breaks.empty())
+        SwPaM temp(rPam, nullptr);
+        if (SwTextNode *const pNode = temp.Start()->nNode.GetNode().GetTextNode())
+        { // rPam may not have nContent set but IsFieldmarkOverlap requires it
+            pNode->MakeStartIndex(&temp.Start()->nContent);
+        }
+        if (SwTextNode *const pNode = temp.End()->nNode.GetNode().GetTextNode())
+        {
+            pNode->MakeEndIndex(&temp.End()->nContent);
+        }
+        if (sw::mark::IsFieldmarkOverlap(temp))
         {   // a bit of a problem: we want to completely remove the nodes
             // but then how can the CH_TXT_ATR survive?
             return false;
@@ -2100,13 +2118,7 @@ bool DocumentContentOperationsManager::MoveRange( SwPaM& rPaM, SwPosition& rPos,
     if( !rPaM.HasMark() || *pStt >= *pEnd || (*pStt <= rPos && rPos < *pEnd))
         return false;
 
-#ifndef NDEBUG
-    {
-        std::vector<std::pair<sal_uLong, sal_Int32>> Breaks;
-        lcl_CalcBreaks(Breaks, rPaM);
-        assert(Breaks.empty()); // probably an invalid redline was created?
-    }
-#endif
+    assert(!sw::mark::IsFieldmarkOverlap(rPaM)); // probably an invalid redline was created?
 
     // Save the paragraph anchored Flys, so that they can be moved.
     SaveFlyArr aSaveFlyArr;
diff --git a/sw/source/core/doc/docbm.cxx b/sw/source/core/doc/docbm.cxx
index c2118fd444f2..c0c973904bf5 100644
--- a/sw/source/core/doc/docbm.cxx
+++ b/sw/source/core/doc/docbm.cxx
@@ -582,6 +582,16 @@ namespace sw { namespace mark
             return nullptr;
         }
 
+        if ((eType == MarkType::TEXT_FIELDMARK || eType == MarkType::DATE_FIELDMARK)
+            // can't check for Copy - it asserts - but it's also obviously unnecessary
+            && eMode == InsertMode::New
+            && sw::mark::IsFieldmarkOverlap(rPaM))
+        {
+            SAL_WARN("sw.core", "MarkManager::makeMark(..)"
+                " - invalid range on fieldmark, overlaps existing fieldmark or meta-field");
+            return nullptr;
+        }
+
         // create mark
         std::unique_ptr<::sw::mark::MarkBase> pMark;
         switch(eType)
diff --git a/sw/source/core/inc/bookmrk.hxx b/sw/source/core/inc/bookmrk.hxx
index cd0e154185db..3960ca4b3d8b 100644
--- a/sw/source/core/inc/bookmrk.hxx
+++ b/sw/source/core/inc/bookmrk.hxx
@@ -339,6 +339,9 @@ namespace sw {
 
         /// return position of the CH_TXT_ATR_FIELDSEP for rMark
         SwPosition FindFieldSep(IFieldmark const& rMark);
+
+        /// check if rPaM is valid range of new fieldmark
+        bool IsFieldmarkOverlap(SwPaM const& rPaM);
     }
 }
 #endif
diff --git a/sw/source/core/undo/rolbck.cxx b/sw/source/core/undo/rolbck.cxx
index 6f0c1de92bb1..ef4815a1cff4 100644
--- a/sw/source/core/undo/rolbck.cxx
+++ b/sw/source/core/undo/rolbck.cxx
@@ -744,11 +744,13 @@ void SwHistoryTextFieldmark::SetInDoc(SwDoc* pDoc, bool)
 
     SwPaM const pam(*rNds[m_nStartNode]->GetContentNode(), m_nStartContent,
                     *rNds[m_nEndNode]->GetContentNode(),
+                        // subtract 1 for the CH_TXT_ATR_FIELDEND itself,
+                        // plus more if same node as other CH_TXT_ATR
                         m_nStartNode == m_nEndNode
-                            ? (m_nEndContent - 2)
+                            ? (m_nEndContent - 3)
                             : m_nSepNode == m_nEndNode
-                                ? (m_nEndContent - 1)
-                                : m_nEndContent);
+                                ? (m_nEndContent - 2)
+                                : (m_nEndContent - 1));
     SwPosition const sepPos(*rNds[m_nSepNode]->GetContentNode(),
             m_nStartNode == m_nSepNode ? (m_nSepContent - 1) : m_nSepContent);