sc: convert remaining raw ScPatternAttr* class members to CellAttributeHolder

We have added various workarounds for ScPatternAttr use after free issues since: commit 2e1f9da8a6 Date: Sat Dec 23 15:52:06 2023 +0100 Decouple ScPatternAttr from SfxItemPool separated ScPatternAttr from SfxItemPool. Prior to that an ScPatternAttr was an SfxPoolItem held in the doc's SfxItemPool which kept every item alive for the document's whole lifetime, so any raw const ScPatternAttr* was basically safe. The various workarounds for the new behaviour were: commit dca19993a4c732e95b9a47e1abdcaad6efbeebe9 Date: Sat Jan 6 14:40:28 2024 +0100 tdf#159042: crash after changing cell style (if direct formatting done) commit 19437715eb415fe2c40b8b40dbe04ddca28f9a35 Date: Thu Sep 12 12:06:28 2024 +0100 null pattern deref seen in ScViewData::SetEditEngine commit e384f2e8de9603ee65a3edc6dcb50b07116ee4fc Date: Mon Mar 10 14:45:22 2025 +0000 crashtesting: crash on reimport of xlsx output of forum-pl-2758.ods commit 7999585bd8378f1a8d04ea2a9a93a8e0bf4d8eb6 Date: Fri May 9 08:50:17 2025 +0100 crash on null pattern de-ref commit 1bf9bbea96 Date: Tue Oct 28 16:53:49 2025 +0300 tdf#169115: sc: fix use-after-free bug in ScDocument::FillInfo commit 9f366af81478ad218dc2ef2e37423a9ebcc84e73 Date: Sat Apr 25 14:47:02 2026 +0200 ScPatternAttr use-after-free commit 20fe691a18fb35120d0765dd1519acd6d2986c38 Date: Tue Apr 28 08:33:34 2026 +0200 ScPatternAttr use-after-free (2) This commit picks the class member fields that are still raw ScPatternAttr* and converts them to CellAttributeHolder: ScUsedAreaIterator::pPattern and pFoundPattern ScInputHandler::pLastPattern LotAttrCol::ENTRY::pPattAttr ScPatternCache::Entry::pPattern The CalcAll() workaround from 9f366af81478 then doesn't seem to be needed(?) A new compiler plugin catches class member fields of raw type ScPatternAttr pointer. Signed-off-by: Caolán McNamara <caolan.mcnamara@collabora.com> Change-Id: Ibb9f28b8fcaa5470c7a82eee52900c2d60434e41 Reviewed-on: https://gerrit.collaboraoffice.com/c/online/+/3163 Tested-by: Jenkins CPCI <releng@collaboraoffice.com> Reviewed-by: Noel Grandin <noel.grandin@collabora.com> (cherry picked from commit 9c7b6313a29067bf0eef12884fc3ce7efb0374e5) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/209822 Reviewed-by: Xisco Fauli <xiscofauli@libreoffice.org> Tested-by: Jenkins
2026-08-25 08:52:44 +02:00
parent 45225d31c4
commit 8749466df2
11 changed files with 180 additions and 47 deletions
@@ -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 <set>
+#include <string>
+
+#include "config_clang.h"
+
+#include "check.hxx"
+#include "compat.hxx"
+#include "plugin.hxx"
+
+namespace
+{
+class ScPatternOwnership final : public loplugin::FilteringPlugin<ScPatternOwnership>
+{
+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<CXXRecordDecl>(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<RecordType>();
+        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("scpatternownership");
+}
+
+#endif // LO_CLANG_SHARED_PLUGINS
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */