Fix null-deref thanks to an attribute on a global declarator chunk #83611

erichkeane · 2024-03-01T19:42:13Z

This was reported (sort of) in a PR: #77703. The problem is that a declarator 'owns' an attributes allocation via an AttributePool. However, this example tries to copy a DeclaratorChunk from one Declarator to another, so when the temporary Declarator goes out of scope, it deletes the attribute it has tried to pass on via the chunk.

This patch ensures that we copy the 'ownership' of the attribute correctly, and adds an assert to catch any other casess where this happens.

Additionally, this was put in as a bug report, so this
Fixes #83611

This was reported (sort of) in a PR: llvm#77703. The problem is that a declarator 'owns' an attributes allocation via an `AttributePool`. However, this example tries to copy a DeclaratorChunk from one Declarator to another, so when the temporary Declarator goes out of scope, it deletes the attribute it has tried to pass on via the chunk. This patch ensures that we copy the 'ownership' of the attribute correctly, and adds an assert to catch any other casess where this happens.

llvmbot · 2024-03-01T19:42:42Z

@llvm/pr-subscribers-clang

Author: Erich Keane (erichkeane)

Changes

This was reported (sort of) in a PR: #77703. The problem is that a declarator 'owns' an attributes allocation via an AttributePool. However, this example tries to copy a DeclaratorChunk from one Declarator to another, so when the temporary Declarator goes out of scope, it deletes the attribute it has tried to pass on via the chunk.

This patch ensures that we copy the 'ownership' of the attribute correctly, and adds an assert to catch any other casess where this happens.

Full diff: https://github.com/llvm/llvm-project/pull/83611.diff

6 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+4-1)
(modified) clang/include/clang/Sema/DeclSpec.h (+16)
(modified) clang/include/clang/Sema/ParsedAttr.h (+5)
(modified) clang/lib/Parse/ParseDecl.cpp (+1-1)
(modified) clang/lib/Sema/ParsedAttr.cpp (+6)
(added) clang/test/Parser/cxx-declarator-attribute-crash.cpp (+8)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1286263700963c..3476342ef6f6e4 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -306,7 +306,10 @@ Bug Fixes to C++ Support
   instead of only on class, alias, and variable templates, as last updated by
   CWG2032.
   Fixes (`#83461 <https://github.com/llvm/llvm-project/issues/83461>`_)
-
+- Fixed an issue where an attribute on a declarator would cause the attribute to
+  be destructed prematurely. This fixes a pair of Chromium that were brought to
+  our attention by an attempt to fix in
+  `#77703 <https://github.com/llvm/llvm-project/pull/77703>`_.
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/DeclSpec.h b/clang/include/clang/Sema/DeclSpec.h
index 316e8071169a3a..a176159707486c 100644
--- a/clang/include/clang/Sema/DeclSpec.h
+++ b/clang/include/clang/Sema/DeclSpec.h
@@ -2359,11 +2359,27 @@ class Declarator {
       SetRangeEnd(EndLoc);
   }
 
+  /// AddTypeInfo - Add a chunk to this declarator. Also extend the range to
+  /// EndLoc, which should be the last token of the chunk. This overload is for
+  /// copying a 'chunk' from another declarator, so it takes the pool that the
+  /// other Declarator owns so that it can 'take' the attributes from it.
+  void AddTypeInfo(const DeclaratorChunk &TI, AttributePool &OtherPool,
+                   SourceLocation EndLoc) {
+    DeclTypeInfo.push_back(TI);
+    getAttributePool().takeFrom(DeclTypeInfo.back().getAttrs(), OtherPool);
+
+    if (!EndLoc.isInvalid())
+      SetRangeEnd(EndLoc);
+  }
+
   /// AddTypeInfo - Add a chunk to this declarator. Also extend the range to
   /// EndLoc, which should be the last token of the chunk.
   void AddTypeInfo(const DeclaratorChunk &TI, SourceLocation EndLoc) {
     DeclTypeInfo.push_back(TI);
 
+    assert(TI.AttrList.empty() &&
+           "Cannot add a declarator chunk with attributes with this overload");
+
     if (!EndLoc.isInvalid())
       SetRangeEnd(EndLoc);
   }
diff --git a/clang/include/clang/Sema/ParsedAttr.h b/clang/include/clang/Sema/ParsedAttr.h
index 8c3ba39031aad8..53cc76da34df27 100644
--- a/clang/include/clang/Sema/ParsedAttr.h
+++ b/clang/include/clang/Sema/ParsedAttr.h
@@ -680,6 +680,7 @@ class AttributeFactory {
   ~AttributeFactory();
 };
 
+class ParsedAttributesView;
 class AttributePool {
   friend class AttributeFactory;
   friend class ParsedAttributes;
@@ -734,6 +735,9 @@ class AttributePool {
     pool.Attrs.clear();
   }
 
+  /// Take a list of attributes from another pool and add them to this pool.
+  void takeFrom(ParsedAttributesView &List, AttributePool &Pool);
+
   ParsedAttr *create(IdentifierInfo *attrName, SourceRange attrRange,
                      IdentifierInfo *scopeName, SourceLocation scopeLoc,
                      ArgsUnion *args, unsigned numArgs, ParsedAttr::Form form,
@@ -816,6 +820,7 @@ class AttributePool {
 };
 
 class ParsedAttributesView {
+  friend class AttributePool;
   using VecTy = llvm::SmallVector<ParsedAttr *>;
   using SizeType = decltype(std::declval<VecTy>().size());
 
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index edfab11c37cf0f..ccbfea6a66fba7 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -7928,7 +7928,7 @@ void Parser::ParseMisplacedBracketDeclarator(Declarator &D) {
   // Adding back the bracket info to the end of the Declarator.
   for (unsigned i = 0, e = TempDeclarator.getNumTypeObjects(); i < e; ++i) {
     const DeclaratorChunk &Chunk = TempDeclarator.getTypeObject(i);
-    D.AddTypeInfo(Chunk, SourceLocation());
+    D.AddTypeInfo(Chunk, TempDeclarator.getAttributePool(), SourceLocation());
   }
 
   // The missing identifier would have been diagnosed in ParseDirectDeclarator.
diff --git a/clang/lib/Sema/ParsedAttr.cpp b/clang/lib/Sema/ParsedAttr.cpp
index 06c213267c7ef5..978d4e7bf4d392 100644
--- a/clang/lib/Sema/ParsedAttr.cpp
+++ b/clang/lib/Sema/ParsedAttr.cpp
@@ -100,6 +100,12 @@ void AttributePool::takePool(AttributePool &pool) {
   pool.Attrs.clear();
 }
 
+void AttributePool::takeFrom(ParsedAttributesView &List, AttributePool &Pool) {
+    assert(&Pool != this && "AttributePool can't take attributes from itself");
+    llvm::for_each(List.AttrList, [&Pool](ParsedAttr *A) { Pool.remove(A); });
+    Attrs.insert(Attrs.end(), List.AttrList.begin(), List.AttrList.end());
+}
+
 namespace {
 
 #include "clang/Sema/AttrParsedAttrImpl.inc"
diff --git a/clang/test/Parser/cxx-declarator-attribute-crash.cpp b/clang/test/Parser/cxx-declarator-attribute-crash.cpp
new file mode 100644
index 00000000000000..3b989a659db569
--- /dev/null
+++ b/clang/test/Parser/cxx-declarator-attribute-crash.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// expected-error@+5{{brackets are not allowed here}}
+// expected-error@+4{{a type specifier is required for all declarations}}
+// expected-warning@+3{{unknown attribute 'h' ignored}}
+// expected-error@+2{{definition of variable with array type}}
+// expected-error@+1{{expected ';'}}
+[][[h]]l

github-actions · 2024-03-01T19:44:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

shafik

Note, I opened an issue for this here: #83385

shafik · 2024-03-02T04:29:27Z

clang/lib/Sema/ParsedAttr.cpp

@@ -100,6 +100,12 @@ void AttributePool::takePool(AttributePool &pool) {
  pool.Attrs.clear();
 }

+void AttributePool::takeFrom(ParsedAttributesView &List, AttributePool &Pool) {
+  assert(&Pool != this && "AttributePool can't take attributes from itself");
+  llvm::for_each(List.AttrList, [&Pool](ParsedAttr *A) { Pool.remove(A); });


Should we remove from the pool after we added them to the Attrs?

You mean reverse the order of the remove/add? Based on takeAllFrom and takeOneFrom (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/ParsedAttr.h#L953), we are consistently inconsistent :)

In this case, I was working from takeOneFrom so I ended up in this order, but if there is a good reason to prefer one over the other, I can switch.

erichkeane · 2024-03-02T15:55:08Z

Note to self when I get back to work:
1- Update commit message with bug # 83611
2- Add bug # to release note.

erichkeane · 2024-03-04T14:15:03Z

Note to self when I get back to work:

Both done now!

AaronBallman

LGTM aside from a tiny commenting nit (take it or leave it).

AaronBallman · 2024-03-04T14:31:37Z

clang/include/clang/Sema/ParsedAttr.h

@@ -734,6 +735,9 @@ class AttributePool {
    pool.Attrs.clear();
  }

+  /// Take a list of attributes from another pool and add them to this pool.


Suggested change

/// Take a list of attributes from another pool and add them to this pool.

/// Removes the attributes from \c List, which are owned by \c Pool, and adds them at the end

/// of this \c AttributePool.

erichkeane requested review from AaronBallman and shafik March 1, 2024 19:42

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 1, 2024

clang-format

3f05e51

shafik reviewed Mar 2, 2024

View reviewed changes

erichkeane added 2 commits March 4, 2024 06:10

Update release note to include Shafik's bug report

28c0b7d

Merge branch 'main' into PR77703

5542509

Merge branch 'main' into PR77703

885480b

AaronBallman approved these changes Mar 4, 2024

View reviewed changes

Update comment suggested by Aaron

da48f02

shafik approved these changes Mar 4, 2024

View reviewed changes

erichkeane merged commit bd7bce2 into llvm:main Mar 4, 2024

erichkeane mentioned this pull request Mar 4, 2024

Malformed attribute results in crash #83385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix null-deref thanks to an attribute on a global declarator chunk #83611

Fix null-deref thanks to an attribute on a global declarator chunk #83611

Uh oh!

erichkeane commented Mar 1, 2024 •

edited

Loading

Uh oh!

llvmbot commented Mar 1, 2024

Uh oh!

github-actions bot commented Mar 1, 2024 •

edited

Loading

Uh oh!

shafik left a comment

Uh oh!

shafik Mar 2, 2024

Uh oh!

erichkeane Mar 2, 2024

Uh oh!

erichkeane commented Mar 2, 2024

Uh oh!

erichkeane commented Mar 4, 2024

Uh oh!

AaronBallman left a comment

Uh oh!

AaronBallman Mar 4, 2024

Uh oh!

Uh oh!

	/// Take a list of attributes from another pool and add them to this pool.
	/// Removes the attributes from \c List, which are owned by \c Pool, and adds them at the end
	/// of this \c AttributePool.

Fix null-deref thanks to an attribute on a global declarator chunk #83611

Fix null-deref thanks to an attribute on a global declarator chunk #83611

Uh oh!

Conversation

erichkeane commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 1, 2024

Uh oh!

github-actions bot commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

shafik Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Mar 2, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Mar 2, 2024

Uh oh!

erichkeane commented Mar 4, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erichkeane commented Mar 1, 2024 •

edited

Loading

github-actions bot commented Mar 1, 2024 •

edited

Loading