sw/qa/core/data/ww8/pass/ofz18554-1.doc |binary sw/qa/extras/ooxmlexport/data/tdf128889.fodt | 15 +++++++ sw/qa/extras/ooxmlexport/ooxmlexport14.cxx | 11 +++++ sw/source/core/doc/DocumentContentOperationsManager.cxx | 32 +++++++++++----- sw/source/core/doc/docbm.cxx | 10 +++++ sw/source/core/inc/bookmrk.hxx | 3 + sw/source/core/undo/rolbck.cxx | 8 ++-- sw/source/filter/ww8/attributeoutputbase.hxx | 3 + sw/source/filter/ww8/docxattributeoutput.cxx | 21 ++++++++-- sw/source/filter/ww8/docxattributeoutput.hxx | 6 ++- sw/source/filter/ww8/docxexport.cxx | 4 +- sw/source/filter/ww8/rtfattributeoutput.cxx | 3 + sw/source/filter/ww8/rtfattributeoutput.hxx | 3 + sw/source/filter/ww8/rtfexport.cxx | 4 +- sw/source/filter/ww8/ww8atr.cxx | 8 ++-- sw/source/filter/ww8/ww8attributeoutput.hxx | 2 - 16 files changed, 103 insertions(+), 30 deletions(-)
New commits: commit bd2ada701aad2c4e85d03cd8db68eaeae081d91c Author: Michael Stahl <[email protected]> AuthorDate: Tue Nov 19 15:39:33 2019 +0100 Commit: Caolán McNamara <[email protected]> CommitDate: Tue Nov 19 22:18:30 2019 +0100 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 <[email protected]> Tested-by: Caolán McNamara <[email protected]> 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 Binary files /dev/null and b/sw/qa/core/data/ww8/pass/ofz18554-1.doc differ 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); commit b0e7e494b6bc69d3833c0a6c256ff8106a4a24cb Author: Mike Kaganski <[email protected]> AuthorDate: Tue Nov 19 22:41:52 2019 +0300 Commit: Mike Kaganski <[email protected]> CommitDate: Tue Nov 19 22:18:27 2019 +0100 tdf#128889: don't write "page break after" into w:pPr This produced invalid OOXML, which Word considers as "page before", and LibreOffice ignores when re-importing. Make sure to write it as *trailing* w:r with w:br, as Word also does when imports ODT with this atribute, and saves as DOCX. Change-Id: Ifc4f45d65d4455ecb5cd62aed1ef6a03375c8aa4 Reviewed-on: https://gerrit.libreoffice.org/83232 Tested-by: Jenkins Reviewed-by: Mike Kaganski <[email protected]> diff --git a/sw/qa/extras/ooxmlexport/data/tdf128889.fodt b/sw/qa/extras/ooxmlexport/data/tdf128889.fodt new file mode 100644 index 000000000000..6dc1c4202696 --- /dev/null +++ b/sw/qa/extras/ooxmlexport/data/tdf128889.fodt @@ -0,0 +1,15 @@ +<?xml version="1.0" encoding="UTF-8"?> + +<office:document xmlns:office="urn:oasis:names:tc:opendocument:xmlns:office:1.0" xmlns:fo="urn:oasis:names:tc:opendocument:xmlns:xsl-fo-compatible:1.0" xmlns:style="urn:oasis:names:tc:opendocument:xmlns:style:1.0" xmlns:text="urn:oasis:names:tc:opendocument:xmlns:text:1.0" office:version="1.2" office:mimetype="application/vnd.oasis.opendocument.text"> + <office:automatic-styles> + <style:style style:name="P1" style:family="paragraph" style:parent-style-name="Standard"> + <style:paragraph-properties fo:break-after="page"/> + </style:style> + </office:automatic-styles> + <office:body> + <office:text> + <text:p text:style-name="P1">para1</text:p> + <text:p>para2</text:p> + </office:text> + </office:body> +</office:document> \ No newline at end of file diff --git a/sw/qa/extras/ooxmlexport/ooxmlexport14.cxx b/sw/qa/extras/ooxmlexport/ooxmlexport14.cxx index 8996ba0edad7..3bafcab32d6b 100644 --- a/sw/qa/extras/ooxmlexport/ooxmlexport14.cxx +++ b/sw/qa/extras/ooxmlexport/ooxmlexport14.cxx @@ -168,6 +168,17 @@ DECLARE_OOXMLEXPORT_EXPORTONLY_TEST(testTdf128820, "tdf128820.fodt") "a:graphic/a:graphicData/wpg:wgp/wps:wsp"); } +DECLARE_OOXMLEXPORT_EXPORTONLY_TEST(testTdf128889, "tdf128889.fodt") +{ + xmlDocPtr pXml = parseExport("word/document.xml"); + CPPUNIT_ASSERT(pXml); + // There was an w:r (with w:br) as an invalid child of first paragraph's w:pPr + assertXPath(pXml, "/w:document/w:body/w:p[1]/w:pPr/w:r", 0); + assertXPath(pXml, "/w:document/w:body/w:p[1]/w:r", 2); + // Check that the break is in proper - last - position + assertXPath(pXml, "/w:document/w:body/w:p[1]/w:r[2]/w:br", "type", "page"); +} + CPPUNIT_PLUGIN_IMPLEMENT(); /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sw/source/filter/ww8/attributeoutputbase.hxx b/sw/source/filter/ww8/attributeoutputbase.hxx index 74084f625590..70509ed47806 100644 --- a/sw/source/filter/ww8/attributeoutputbase.hxx +++ b/sw/source/filter/ww8/attributeoutputbase.hxx @@ -299,7 +299,8 @@ public: /// Write a section break /// msword::ColumnBreak or msword::PageBreak - virtual void SectionBreak( sal_uInt8 nC, const WW8_SepInfo* pSectionInfo = nullptr ) = 0; + /// bBreakAfter: the break must be scheduled for insertion in the end of current paragraph + virtual void SectionBreak( sal_uInt8 nC, bool bBreakAfter, const WW8_SepInfo* pSectionInfo = nullptr ) = 0; // preserve page vertical alignment virtual void TextVerticalAdjustment( const css::drawing::TextVerticalAdjust) {}; diff --git a/sw/source/filter/ww8/docxattributeoutput.cxx b/sw/source/filter/ww8/docxattributeoutput.cxx index 3c2c614bf096..4f18582b0d7f 100644 --- a/sw/source/filter/ww8/docxattributeoutput.cxx +++ b/sw/source/filter/ww8/docxattributeoutput.cxx @@ -745,6 +745,13 @@ void DocxAttributeOutput::EndParagraph( ww8::WW8TableNodeInfoInner::Pointer_t pT m_bStartedCharSdt = false; } + if (m_bPageBreakAfter) + { + // tdf#128889 Trailing page break + SectionBreak(msword::PageBreak, false); + m_bPageBreakAfter = false; + } + m_pSerializer->endElementNS( XML_w, XML_p ); // on export sdt blocks are never nested ATM if( !m_bAnchorLinkedToNode && !m_bStartedParaSdt ) @@ -5981,7 +5988,7 @@ void DocxAttributeOutput::PageBreakBefore( bool bBreak ) FSNS( XML_w, XML_val ), "false" ); } -void DocxAttributeOutput::SectionBreak( sal_uInt8 nC, const WW8_SepInfo* pSectionInfo ) +void DocxAttributeOutput::SectionBreak( sal_uInt8 nC, bool bBreakAfter, const WW8_SepInfo* pSectionInfo ) { switch ( nC ) { @@ -6043,9 +6050,15 @@ void DocxAttributeOutput::SectionBreak( sal_uInt8 nC, const WW8_SepInfo* pSectio } else if ( m_bParagraphOpened ) { - m_pSerializer->startElementNS(XML_w, XML_r); - m_pSerializer->singleElementNS(XML_w, XML_br, FSNS(XML_w, XML_type), "page"); - m_pSerializer->endElementNS( XML_w, XML_r ); + if (bBreakAfter) + // tdf#128889 + m_bPageBreakAfter = true; + else + { + m_pSerializer->startElementNS(XML_w, XML_r); + m_pSerializer->singleElementNS(XML_w, XML_br, FSNS(XML_w, XML_type), "page"); + m_pSerializer->endElementNS(XML_w, XML_r); + } } else m_bPostponedPageBreak = true; diff --git a/sw/source/filter/ww8/docxattributeoutput.hxx b/sw/source/filter/ww8/docxattributeoutput.hxx index 2a8a43a76a79..67561087ceb3 100644 --- a/sw/source/filter/ww8/docxattributeoutput.hxx +++ b/sw/source/filter/ww8/docxattributeoutput.hxx @@ -270,7 +270,8 @@ public: /// Write a section break /// msword::ColumnBreak or msword::PageBreak - virtual void SectionBreak( sal_uInt8 nC, const WW8_SepInfo* pSectionInfo = nullptr ) override; + /// bBreakAfter: the break must be scheduled for insertion in the end of current paragraph + virtual void SectionBreak( sal_uInt8 nC, bool bBreakAfter, const WW8_SepInfo* pSectionInfo = nullptr ) override; // preserve DOCX page vertical alignment virtual void TextVerticalAdjustment( const css::drawing::TextVerticalAdjust ) override; @@ -841,6 +842,9 @@ private: // beginning of the next paragraph bool m_bPostponedPageBreak; + // This paragraph must end with page break + bool m_bPageBreakAfter = false; + std::vector<ww8::Frame> m_aFramesOfParagraph; std::set<const SwFrameFormat*> m_aFloatingTablesOfParagraph; sal_Int32 m_nTextFrameLevel; diff --git a/sw/source/filter/ww8/docxexport.cxx b/sw/source/filter/ww8/docxexport.cxx index c6226fd130ab..3e9c4bc5fd1b 100644 --- a/sw/source/filter/ww8/docxexport.cxx +++ b/sw/source/filter/ww8/docxexport.cxx @@ -547,7 +547,7 @@ ErrCode DocxExport::ExportDocument_Impl() void DocxExport::AppendSection( const SwPageDesc *pPageDesc, const SwSectionFormat* pFormat, sal_uLong nLnNum ) { - AttrOutput().SectionBreak( msword::PageBreak, m_pSections->CurrentSectionInfo() ); + AttrOutput().SectionBreak( msword::PageBreak, false, m_pSections->CurrentSectionInfo() ); m_pSections->AppendSection( pPageDesc, pFormat, nLnNum, m_pAttrOutput->IsFirstParagraph() ); } @@ -622,7 +622,7 @@ void DocxExport::PrepareNewPageDesc( const SfxItemSet* pSet, { // tell the attribute output that we are ready to write the section // break [has to be output inside paragraph properties] - AttrOutput().SectionBreak( msword::PageBreak, m_pSections->CurrentSectionInfo() ); + AttrOutput().SectionBreak( msword::PageBreak, false, m_pSections->CurrentSectionInfo() ); const SwSectionFormat* pFormat = GetSectionFormat( rNd ); const sal_uLong nLnNm = GetSectionLineNo( pSet, rNd ); diff --git a/sw/source/filter/ww8/rtfattributeoutput.cxx b/sw/source/filter/ww8/rtfattributeoutput.cxx index 67622810d0db..6a04e707a706 100644 --- a/sw/source/filter/ww8/rtfattributeoutput.cxx +++ b/sw/source/filter/ww8/rtfattributeoutput.cxx @@ -1200,7 +1200,8 @@ void RtfAttributeOutput::PageBreakBefore(bool bBreak) } } -void RtfAttributeOutput::SectionBreak(sal_uInt8 nC, const WW8_SepInfo* pSectionInfo) +void RtfAttributeOutput::SectionBreak(sal_uInt8 nC, bool /*bBreakAfter*/, + const WW8_SepInfo* pSectionInfo) { switch (nC) { diff --git a/sw/source/filter/ww8/rtfattributeoutput.hxx b/sw/source/filter/ww8/rtfattributeoutput.hxx index fe0d093ae0a3..4ea8b3845bcd 100644 --- a/sw/source/filter/ww8/rtfattributeoutput.hxx +++ b/sw/source/filter/ww8/rtfattributeoutput.hxx @@ -165,7 +165,8 @@ public: /// Write a section break /// msword::ColumnBreak or msword::PageBreak - void SectionBreak(sal_uInt8 nC, const WW8_SepInfo* pSectionInfo = nullptr) override; + void SectionBreak(sal_uInt8 nC, bool bBreakAfter, + const WW8_SepInfo* pSectionInfo = nullptr) override; /// Start of the section properties. void StartSection() override; diff --git a/sw/source/filter/ww8/rtfexport.cxx b/sw/source/filter/ww8/rtfexport.cxx index 787833bbac71..f29268032ed0 100644 --- a/sw/source/filter/ww8/rtfexport.cxx +++ b/sw/source/filter/ww8/rtfexport.cxx @@ -970,7 +970,7 @@ void RtfExport::PrepareNewPageDesc(const SfxItemSet* pSet, const SwNode& rNd, // Don't insert a page break, when we're changing page style just because the next page has to be a different one. if (!m_pAttrOutput->GetPrevPageDesc() || m_pAttrOutput->GetPrevPageDesc()->GetFollow() != pNewPgDesc) - AttrOutput().SectionBreak(msword::PageBreak, m_pSections->CurrentSectionInfo()); + AttrOutput().SectionBreak(msword::PageBreak, false, m_pSections->CurrentSectionInfo()); } bool RtfExport::DisallowInheritingOutlineNumbering(const SwFormat& rFormat) @@ -1026,7 +1026,7 @@ void RtfExport::AppendSection(const SwPageDesc* pPageDesc, const SwSectionFormat sal_uLong nLnNum) { m_pSections->AppendSection(pPageDesc, pFormat, nLnNum); - AttrOutput().SectionBreak(msword::PageBreak, m_pSections->CurrentSectionInfo()); + AttrOutput().SectionBreak(msword::PageBreak, false, m_pSections->CurrentSectionInfo()); } RtfExport::RtfExport(RtfExportFilter* pFilter, SwDoc* pDocument, diff --git a/sw/source/filter/ww8/ww8atr.cxx b/sw/source/filter/ww8/ww8atr.cxx index b72a0246bf21..f4525e09b663 100644 --- a/sw/source/filter/ww8/ww8atr.cxx +++ b/sw/source/filter/ww8/ww8atr.cxx @@ -2191,7 +2191,7 @@ void AttributeOutputBase::StartTOX( const SwSection& rSect ) SwSection *pParent = rSect.GetParent(); WW8_SepInfo rInfo(&GetExport( ).m_pDoc->GetPageDesc(0), pParent ? pParent->GetFormat() : nullptr, 0/*nRstLnNum*/); - GetExport( ).AttrOutput().SectionBreak( msword::PageBreak, &rInfo ); + GetExport( ).AttrOutput().SectionBreak( msword::PageBreak, false, &rInfo ); } sStr += "\\c \"" + OUString::number( nCol ) + "\""; @@ -2499,7 +2499,7 @@ void AttributeOutputBase::EndTOX( const SwSection& rSect,bool bCareEnd ) if ( 0 < nCol ) { WW8_SepInfo rInfo( &GetExport( ).m_pDoc->GetPageDesc( 0 ), rSect.GetFormat(), 0/*nRstLnNum*/ ); - GetExport( ).AttrOutput().SectionBreak( msword::PageBreak, &rInfo ); + GetExport( ).AttrOutput().SectionBreak( msword::PageBreak, false, &rInfo ); } } } @@ -3880,13 +3880,13 @@ void AttributeOutputBase::FormatBreak( const SvxFormatBreakItem& rBreak ) } if ( !bFollowPageDescWritten ) { - SectionBreak( nC ); + SectionBreak(nC, !bBefore); } } } } -void WW8AttributeOutput::SectionBreak( sal_uInt8 nC, const WW8_SepInfo* /*pSectionInfo*/ ) +void WW8AttributeOutput::SectionBreak( sal_uInt8 nC, bool /*bBreakAfter*/, const WW8_SepInfo* /*pSectionInfo*/ ) { m_rWW8Export.ReplaceCr( nC ); } diff --git a/sw/source/filter/ww8/ww8attributeoutput.hxx b/sw/source/filter/ww8/ww8attributeoutput.hxx index 35d8db7dfa5e..7e3f2a31ff20 100644 --- a/sw/source/filter/ww8/ww8attributeoutput.hxx +++ b/sw/source/filter/ww8/ww8attributeoutput.hxx @@ -147,7 +147,7 @@ public: /// Write a section break /// msword::ColumnBreak or msword::PageBreak - virtual void SectionBreak( sal_uInt8 nC, const WW8_SepInfo* pSectionInfo = nullptr ) override; + virtual void SectionBreak( sal_uInt8 nC, bool bBreakAfter, const WW8_SepInfo* pSectionInfo = nullptr ) override; // preserve DOC page vertical alignment virtual void TextVerticalAdjustment( const css::drawing::TextVerticalAdjust ) override; _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
