diff --git a/compilerplugins/clang/scpatternownership.cxx b/compilerplugins/clang/scpatternownership.cxx new file mode 100644 index 000000000000..b75b1d3a6417 --- /dev/null +++ b/compilerplugins/clang/scpatternownership.cxx @@ -0,0 +1,138 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +// ScPatternAttr ownership rules (canonical source). +// +// ScPatternAttr is the cell-pattern value object owned by a +// CellAttributeHelper. Each ScDocument creates its helper lazily via +// getCellAttributeHelper(); clipboard/undo documents share the helper +// through ScDocument::SharePooledResources. The helper holds a registry +// keyed by style name; each registered ScPatternAttr has a refcount +// tracked by the helper. CellAttributeHolder is the smart-pointer +// wrapper around an ScPatternAttr that bumps the refcount on assign and +// drops it on destruction; doUnregister destroys the pattern when its +// refcount reaches zero. +// +// The rule: any long-lived reference to an ScPatternAttr is held by +// CellAttributeHolder, not by raw pointer. A short-lived raw const +// ScPatternAttr* in a single function scope is fine if no operation +// that can apply a new pattern to a cell (formula interpretation, +// conditional-format evaluation, style change, SetNumberFormat, +// ApplyAttr, broadcasts on cells, ...) happens between the GetPattern() +// call and the use. A raw pointer stored in a class member, or held in +// a loop variable across such an operation, is a use after free waiting +// to happen; for the chain of prior per-call-site workarounds see +// git log --grep='ScPatternAttr.*use-after-free' -- engine/sc/ +// +// This plugin enforces the rule on class member fields: any class field +// of type (const) ScPatternAttr* is flagged. Allow-listed exceptions, +// named individually below: +// - CellAttributeHelper and CellAttributeHolder themselves +// - default-pattern cache fields that point at the helper's default +// ScPatternAttr (lives for the helper's full lifetime) +// - paint-pipeline structs whose lifetime is bounded to one paint +// call: DrawEditParam and the file-local ScDrawStringsVars in +// output2.cxx +// Additions to the allow list need an explicit entry. +// +// Not flagged: function-scope locals and function parameters. The +// plugin is declaration-AST only; flow-sensitive analysis is out of +// scope. Don't reintroduce raw class fields and don't hold raw locals +// across the operations listed above. + +#ifndef LO_CLANG_SHARED_PLUGINS + +#include +#include + +#include "config_clang.h" + +#include "check.hxx" +#include "compat.hxx" +#include "plugin.hxx" + +namespace +{ +class ScPatternOwnership final : public loplugin::FilteringPlugin +{ +public: + explicit ScPatternOwnership(loplugin::InstantiationData const& data) + : FilteringPlugin(data) + { + } + + bool preRun() override { return compiler.getLangOpts().CPlusPlus; } + + void run() override + { + if (preRun()) + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + } + + bool VisitFieldDecl(FieldDecl const* decl) + { + if (ignoreLocation(decl)) + return true; + + // The enclosing class + auto const* parent = dyn_cast(decl->getParent()); + if (!parent) + return true; + + // Skip our two owner-ish classes; they implement the model. + std::string const parentName = parent->getNameAsString(); + if (parentName == "CellAttributeHelper" || parentName == "CellAttributeHolder") + return true; + + std::string const fieldName = decl->getNameAsString(); + + // Default-pattern cache fields whose target is the helper's default + // ScPatternAttr, which lives for the helper's full lifetime. + if (parentName == "ScAttrIterator" && fieldName == "pDefPattern") + return true; + if (parentName == "AttrList" && fieldName == "mpDefPattern") // oox::xls + return true; + + // Paint-pipeline structs whose lifetime is bounded to the call that + // builds them; the pattern is set once, used immediately, and the + // struct goes out of scope long before the document does. The same + // applies to the file-local ScDrawStringsVars in output2.cxx. + if (parentName == "DrawEditParam" + && (fieldName == "mpPattern" || fieldName == "mpOldPattern")) + return true; + if (parentName == "ScDrawStringsVars" && fieldName == "pPattern") + return true; + + // Is it ScPatternAttr* or const ScPatternAttr*? + QualType qt = decl->getType(); + if (!qt->isPointerType()) + return true; + QualType pt = qt->getPointeeType().getUnqualifiedType().getCanonicalType(); + auto* rt = pt->getAs(); + if (!rt) + return true; + std::string const pointeeName = rt->getDecl()->getNameAsString(); + if (pointeeName != "ScPatternAttr") + return true; + + report(DiagnosticsEngine::Warning, + "class member of type %0 in %1; long-lived references to " + "ScPatternAttr must be held by CellAttributeHolder", + decl->getLocation()) + << qt << parentName; + return true; + } +}; + +loplugin::Plugin::Registration scpatternownership("scpatternownership"); +} + +#endif // LO_CLANG_SHARED_PLUGINS + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sc/inc/dociter.hxx b/sc/inc/dociter.hxx index 0737bed223ec..6ff884b2960a 100644 --- a/sc/inc/dociter.hxx +++ b/sc/inc/dociter.hxx @@ -24,6 +24,7 @@ #include "global.hxx" #include "scdllapi.h" #include "cellvalue.hxx" +#include "patattr.hxx" #include "mtvelements.hxx" #include "queryparam.hxx" #include "attarray.hxx" @@ -391,12 +392,12 @@ private: SCCOL nAttrCol1; SCCOL nAttrCol2; SCROW nAttrRow; - const ScPatternAttr* pPattern; + CellAttributeHolder maPattern; SCCOL nFoundStartCol; // results after GetNext SCCOL nFoundEndCol; SCROW nFoundRow; - const ScPatternAttr* pFoundPattern; + CellAttributeHolder maFoundPattern; ScRefCellValue maFoundCell; @@ -410,7 +411,7 @@ public: SCCOL GetStartCol() const { return nFoundStartCol; } SCCOL GetEndCol() const { return nFoundEndCol; } SCROW GetRow() const { return nFoundRow; } - const ScPatternAttr* GetPattern() const { return pFoundPattern; } + const ScPatternAttr* GetPattern() const { return maFoundPattern.getScPatternAttr(); } const ScRefCellValue& GetCell() const { return maFoundCell;} }; diff --git a/sc/inc/table.hxx b/sc/inc/table.hxx index 2576338df316..6dee59c4a311 100644 --- a/sc/inc/table.hxx +++ b/sc/inc/table.hxx @@ -682,7 +682,7 @@ public: void SetDirtyAfterLoad(); void SetDirtyVar(); void SetTableOpDirty( const ScRange& ); - SC_DLLPUBLIC void CalcAll(); + void CalcAll(); void CalcAfterLoad( sc::CompileFormulaContext& rCxt, bool bStartListening ); void CompileAll( sc::CompileFormulaContext& rCxt ); void CompileXML( sc::CompileFormulaContext& rCxt, ScProgress& rProgress ); diff --git a/sc/source/core/data/dociter.cxx b/sc/source/core/data/dociter.cxx index 15ec85c57a3e..cd9defaff086 100644 --- a/sc/source/core/data/dociter.cxx +++ b/sc/source/core/data/dociter.cxx @@ -1524,10 +1524,9 @@ ScUsedAreaIterator::ScUsedAreaIterator( ScDocument& rDocument, SCTAB nTable, , nFoundStartCol( 0 ) , nFoundEndCol( 0 ) , nFoundRow( 0 ) - , pFoundPattern( nullptr ) { pCell = aCellIter.GetNext( nCellCol, nCellRow ); - pPattern = aAttrIter.GetNext( nAttrCol1, nAttrCol2, nAttrRow ); + maPattern.setScPatternAttr(aAttrIter.GetNext( nAttrCol1, nAttrCol2, nAttrRow )); } ScUsedAreaIterator::~ScUsedAreaIterator() @@ -1543,22 +1542,22 @@ bool ScUsedAreaIterator::GetNext() while (pCell && pCell->isEmpty()) pCell = aCellIter.GetNext( nCellCol, nCellRow ); - if ( pPattern && IsGreater( nNextCol, nNextRow, nAttrCol2, nAttrRow ) ) - pPattern = aAttrIter.GetNext( nAttrCol1, nAttrCol2, nAttrRow ); + if ( maPattern && IsGreater( nNextCol, nNextRow, nAttrCol2, nAttrRow ) ) + maPattern.setScPatternAttr(aAttrIter.GetNext( nAttrCol1, nAttrCol2, nAttrRow )); - if ( pPattern && nAttrRow == nNextRow && nAttrCol1 < nNextCol ) + if ( maPattern && nAttrRow == nNextRow && nAttrCol1 < nNextCol ) nAttrCol1 = nNextCol; // Find next area bool bFound = true; bool bUseCell = false; - if ( pCell && pPattern ) + if ( pCell && maPattern ) { if ( IsGreater( nCellCol, nCellRow, nAttrCol1, nAttrRow ) ) // Only attributes at the beginning? { maFoundCell.clear(); - pFoundPattern = pPattern; + maFoundPattern = maPattern; nFoundRow = nAttrRow; nFoundStartCol = nAttrCol1; if ( nCellRow == nAttrRow && nCellCol <= nAttrCol2 ) // Area also contains cell? @@ -1570,20 +1569,20 @@ bool ScUsedAreaIterator::GetNext() { bUseCell = true; if ( nAttrRow == nCellRow && nAttrCol1 == nCellCol ) // Attributes on the cell? - pFoundPattern = pPattern; + maFoundPattern = maPattern; else - pFoundPattern = nullptr; + maFoundPattern.setScPatternAttr(nullptr); } } else if ( pCell ) // Just a cell -> take over right away { - pFoundPattern = nullptr; + maFoundPattern.setScPatternAttr(nullptr); bUseCell = true; // Cell position } - else if ( pPattern ) // Just attributes -> take over right away + else if ( maPattern ) // Just attributes -> take over right away { maFoundCell.clear(); - pFoundPattern = pPattern; + maFoundPattern = maPattern; nFoundRow = nAttrRow; nFoundStartCol = nAttrCol1; nFoundEndCol = nAttrCol2; diff --git a/sc/source/filter/excel/xetable.cxx b/sc/source/filter/excel/xetable.cxx index 4ccc61b8cb0b..94dd1e1ebd42 100644 --- a/sc/source/filter/excel/xetable.cxx +++ b/sc/source/filter/excel/xetable.cxx @@ -42,7 +42,6 @@ #include #include #include -#include #include #include #include @@ -2640,11 +2639,6 @@ XclExpCellTable::XclExpCellTable( const XclExpRoot& rRoot ) : nMaxScRow = nLastUsedScRow; maColInfoBfr.Initialize( nMaxScRow ); - // We have to force everything to calculate, or we might end up triggering recalc while - // we walk the data, and then we get stale ScPatternAttr pointers. - if (ScTable* pTable = rDoc.FetchTable(nScTab)) - pTable->CalcAll(); - // range for cell iterator SCCOL nLastIterScCol = nMaxScCol; SCROW nLastIterScRow = ulimit_cast< SCROW >( nLastUsedScRow, nMaxScRow ); diff --git a/sc/source/filter/inc/lotattr.hxx b/sc/source/filter/inc/lotattr.hxx index 94c88b733b6e..89919339b15c 100644 --- a/sc/source/filter/inc/lotattr.hxx +++ b/sc/source/filter/inc/lotattr.hxx @@ -23,6 +23,7 @@ #include #include +#include class ScPatternAttr; class SvxColorItem; @@ -106,7 +107,7 @@ private: struct ENTRY { - const ScPatternAttr* pPattAttr; + CellAttributeHolder pPattAttr; SCROW nFirstRow; SCROW nLastRow; }; diff --git a/sc/source/filter/inc/patterncache.hxx b/sc/source/filter/inc/patterncache.hxx index 25013a930143..895d768402f8 100644 --- a/sc/source/filter/inc/patterncache.hxx +++ b/sc/source/filter/inc/patterncache.hxx @@ -20,6 +20,7 @@ #pragma once #include +#include class ScPatternAttr; @@ -30,7 +31,7 @@ class ScPatternCache { sal_Int32 nXfId; sal_Int32 nNumFmtId; - ScPatternAttr* pPattern; + CellAttributeHolder pPattern; Entry(); }; diff --git a/sc/source/filter/lotus/lotattr.cxx b/sc/source/filter/lotus/lotattr.cxx index cf1da8283be7..efa84138601a 100644 --- a/sc/source/filter/lotus/lotattr.cxx +++ b/sc/source/filter/lotus/lotattr.cxx @@ -188,13 +188,13 @@ void LotAttrCol::SetAttr( const ScDocument* pDoc, const SCROW nRow, const ScPatt if(iterLast != aEntries.rend()) { - if( ( (*iterLast)->nLastRow == nRow - 1 ) && ScPatternAttr::areSame( &rAttr, (*iterLast)->pPattAttr ) ) + if( ( (*iterLast)->nLastRow == nRow - 1 ) && ScPatternAttr::areSame( &rAttr, (*iterLast)->pPattAttr.getScPatternAttr() ) ) (*iterLast)->nLastRow = nRow; else { ENTRY *pCurrent = new ENTRY; - pCurrent->pPattAttr = &rAttr; + pCurrent->pPattAttr.setScPatternAttr(&rAttr); pCurrent->nFirstRow = pCurrent->nLastRow = nRow; aEntries.push_back(std::unique_ptr(pCurrent)); @@ -203,7 +203,7 @@ void LotAttrCol::SetAttr( const ScDocument* pDoc, const SCROW nRow, const ScPatt else { // first entry ENTRY *pCurrent = new ENTRY; - pCurrent->pPattAttr = &rAttr; + pCurrent->pPattAttr.setScPatternAttr(&rAttr); pCurrent->nFirstRow = pCurrent->nLastRow = nRow; aEntries.push_back(std::unique_ptr(pCurrent)); @@ -217,7 +217,7 @@ void LotAttrCol::Apply(LotusContext& rContext, const SCCOL nColNum, const SCTAB for (const auto& rxEntry : aEntries) { rDoc.ApplyPatternAreaTab(nColNum, rxEntry->nFirstRow, nColNum, rxEntry->nLastRow, - nTabNum, *(rxEntry->pPattAttr)); + nTabNum, *(rxEntry->pPattAttr.getScPatternAttr())); } } diff --git a/sc/source/filter/oox/patterncache.cxx b/sc/source/filter/oox/patterncache.cxx index 2431a36fed20..028d4c72321e 100644 --- a/sc/source/filter/oox/patterncache.cxx +++ b/sc/source/filter/oox/patterncache.cxx @@ -22,7 +22,6 @@ ScPatternCache::Entry::Entry() : nXfId(-1) , nNumFmtId(-1) - , pPattern(nullptr) { } @@ -36,7 +35,7 @@ ScPatternAttr* ScPatternCache::query(sal_Int32 nXfId, sal_Int32 nNumFmtId) const for (const auto& entry : maEntries) { if (entry.nXfId == nXfId && entry.nNumFmtId == nNumFmtId) - return entry.pPattern; + return const_cast(entry.pPattern.getScPatternAttr()); } return nullptr; @@ -48,5 +47,5 @@ void ScPatternCache::add(sal_Int32 nXfId, sal_Int32 nNumFmtId, ScPatternAttr* pP nNextPos = ((nNextPos + 1) % nPatternCacheSize); rEntry.nXfId = nXfId; rEntry.nNumFmtId = nNumFmtId; - rEntry.pPattern = pPattern; + rEntry.pPattern.setScPatternAttr(pPattern); } diff --git a/sc/source/ui/app/inputhdl.cxx b/sc/source/ui/app/inputhdl.cxx index a67d7fa5d31b..4ce4b604dbe2 100644 --- a/sc/source/ui/app/inputhdl.cxx +++ b/sc/source/ui/app/inputhdl.cxx @@ -859,8 +859,7 @@ ScInputHandler::ScInputHandler() eAttrAdjust( SvxCellHorJustify::Standard ), fScaleX( 1.0 ), fScaleY( 1.0 ), - pRefViewSh( nullptr ), - pLastPattern( nullptr ) + pRefViewSh( nullptr ) { // The InputHandler is constructed with the view, so SfxViewShell::Current // doesn't have the right view yet. pActiveViewSh is updated in NotifyChange. @@ -999,7 +998,7 @@ void ScInputHandler::UpdateSpellSettings( bool bFromStartTab ) else nCntrl &= ~EEControlBits::ONLINESPELLING; // No AutoCorrect for Symbol Font (EditEngine does no evaluate Default) - if ( pLastPattern && pLastPattern->IsSymbolFont() ) + if ( maLastPattern && maLastPattern.getScPatternAttr()->IsSymbolFont() ) nCntrl &= ~EEControlBits::AUTOCORRECT; else nCntrl |= EEControlBits::AUTOCORRECT; @@ -1019,7 +1018,7 @@ void ScInputHandler::UpdateSpellSettings( bool bFromStartTab ) mpEditEngine->SetSpeller( xXSpellChecker1 ); } - bool bHyphen = pLastPattern && pLastPattern->GetItem(ATTR_HYPHENATE).GetValue(); + bool bHyphen = maLastPattern && maLastPattern.getScPatternAttr()->GetItem(ATTR_HYPHENATE).GetValue(); if ( bHyphen ) { css::uno::Reference xXHyphenator( LinguMgr::GetHyphenator() ); mpEditEngine->SetHyphenator( xXHyphenator ); @@ -2320,7 +2319,7 @@ void ScInputHandler::ViewShellGone(const ScTabViewShell* pViewSh) // Executed sy if ( pViewSh == pActiveViewSh ) { pLastState.reset(); - pLastPattern = nullptr; + maLastPattern.setScPatternAttr(nullptr); } ScModule* mod = ScModule::get(); @@ -2423,7 +2422,7 @@ EditView* ScInputHandler::GetActiveView() void ScInputHandler::ForgetLastPattern() { - pLastPattern = nullptr; + maLastPattern.setScPatternAttr(nullptr); if ( !pLastState && pActiveViewSh ) pActiveViewSh->UpdateInputHandler( true ); // Get status again else @@ -2491,9 +2490,9 @@ void ScInputHandler::UpdateAdjust( sal_Unicode cTyped ) } // Cells with an explicit RTL writing direction are always right adjusted - if (pLastPattern) + if (maLastPattern) { - SvxFrameDirection eDir = pLastPattern->GetItem(ATTR_WRITINGDIR).GetValue(); + SvxFrameDirection eDir = maLastPattern.getScPatternAttr()->GetItem(ATTR_WRITINGDIR).GetValue(); if (eDir == SvxFrameDirection::Horizontal_RL_TB) { eSvxAdjust = SvxAdjust::Right; @@ -2532,9 +2531,9 @@ void ScInputHandler::UpdateAdjust( sal_Unicode cTyped ) break; } - bool bAsianVertical = pLastPattern && - pLastPattern->GetItem( ATTR_STACKED ).GetValue() && - pLastPattern->GetItem( ATTR_VERTICAL_ASIAN ).GetValue(); + bool bAsianVertical = maLastPattern && + maLastPattern.getScPatternAttr()->GetItem( ATTR_STACKED ).GetValue() && + maLastPattern.getScPatternAttr()->GetItem( ATTR_VERTICAL_ASIAN ).GetValue(); if ( bAsianVertical ) { // Always edit at top of cell -> LEFT when editing vertically @@ -2660,7 +2659,7 @@ bool ScInputHandler::StartTable(sal_Unicode cTyped, bool bFromCommand, bool bInp const ScPatternAttr* pPattern = rDoc.GetPattern( aCursorPos.Col(), aCursorPos.Row(), aCursorPos.Tab() ); - if (!ScPatternAttr::areSame(pPattern, pLastPattern)) + if (!ScPatternAttr::areSame(pPattern, maLastPattern.getScPatternAttr())) { // Percent format? const SfxItemSet& rAttrSet = pPattern->GetItemSet(); @@ -2693,7 +2692,7 @@ bool ScInputHandler::StartTable(sal_Unicode cTyped, bool bFromCommand, bool bInp pPattern->FillEditItemSet( pEditDefaults.get() ); mpEditEngine->SetDefaults( *pEditDefaults ); - pLastPattern = pPattern; + maLastPattern.setScPatternAttr(pPattern); bLastIsSymbol = pPattern->IsSymbolFont(); // Background color must be known for automatic font color. @@ -3544,8 +3543,8 @@ void ScInputHandler::EnterHandler2(ScEnterMode nBlockMode, bool bForget, OUStrin StopInputWinEngine(true); // Text input (through number formats) or ApplySelectionPattern modify - // the cell's attributes, so pLastPattern is no longer valid - pLastPattern = nullptr; + // the cell's attributes, so the last pattern is no longer valid + maLastPattern.setScPatternAttr(nullptr); if (bOldMod && !bProtected && !bForget) { diff --git a/sc/source/ui/inc/inputhdl.hxx b/sc/source/ui/inc/inputhdl.hxx index 54d4d946c12f..e30f2c683f64 100644 --- a/sc/source/ui/inc/inputhdl.hxx +++ b/sc/source/ui/inc/inputhdl.hxx @@ -30,6 +30,7 @@ #include #include #include "viewdata.hxx" +#include #include #include @@ -119,7 +120,7 @@ private: ScTabViewShell* pRefViewSh; ScTabViewShell* pActiveViewSh; - const ScPatternAttr* pLastPattern; + CellAttributeHolder maLastPattern; std::unique_ptr pEditDefaults;