From eba82fe884767c65d881c0f636fd0ecebfd64ca8 Mon Sep 17 00:00:00 2001
From: Erik Pilkington <erik.pilkington@gmail.com>
Date: Tue, 2 Apr 2019 19:48:11 +0000
Subject: [PATCH] [Sema] Fix a use-after-deallocate of a ParsedAttr

moveAttrFromListToList only makes sense when moving an attribute to a list with
a pool that's either equivalent, or has a shorter lifetime. Therefore, using it
to move a ParsedAttr from a declarator to a declaration specifier doesn't make
sense, since the declaration specifier's pool outlives the declarator's. The
patch adds a new function, ParsedAttributes::takeOneFrom, which transfers the
attribute from one pool to another, fixing the use-after-deallocate.

rdar://49175426

Differential revision: https://reviews.llvm.org/D60101

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@357516 91177308-0d34-0410-b5e6-96231b3b80d8
---
 include/clang/Sema/ParsedAttr.h         | 8 ++++++++
 lib/Sema/SemaType.cpp                   | 4 ++--
 test/SemaObjC/arc-property-decl-attrs.m | 4 ++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/clang/Sema/ParsedAttr.h b/include/clang/Sema/ParsedAttr.h
index d125d3b806..2e0efe452a 100644
--- a/include/clang/Sema/ParsedAttr.h
+++ b/include/clang/Sema/ParsedAttr.h
@@ -659,6 +659,7 @@ public:
 
 class AttributePool {
   friend class AttributeFactory;
+  friend class ParsedAttributes;
   AttributeFactory &Factory;
   llvm::TinyPtrVector<ParsedAttr *> Attrs;
 
@@ -892,6 +893,13 @@ public:
     pool.takeAllFrom(attrs.pool);
   }
 
+  void takeOneFrom(ParsedAttributes &Attrs, ParsedAttr *PA) {
+    Attrs.getPool().remove(PA);
+    Attrs.remove(PA);
+    getPool().add(PA);
+    addAtEnd(PA);
+  }
+
   void clear() {
     clearListOnly();
     pool.clear();
diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp
index ba028b07e3..3d3b771125 100644
--- a/lib/Sema/SemaType.cpp
+++ b/lib/Sema/SemaType.cpp
@@ -534,8 +534,8 @@ static void distributeObjCPointerTypeAttrFromDeclarator(
       // attribute from being applied multiple times and gives
       // the source-location-filler something to work with.
       state.saveDeclSpecAttrs();
-      moveAttrFromListToList(attr, declarator.getAttributes(),
-                             declarator.getMutableDeclSpec().getAttributes());
+      declarator.getMutableDeclSpec().getAttributes().takeOneFrom(
+          declarator.getAttributes(), &attr);
       return;
     }
   }
diff --git a/test/SemaObjC/arc-property-decl-attrs.m b/test/SemaObjC/arc-property-decl-attrs.m
index 6638054bef..833998d425 100644
--- a/test/SemaObjC/arc-property-decl-attrs.m
+++ b/test/SemaObjC/arc-property-decl-attrs.m
@@ -287,3 +287,7 @@ __attribute__((objc_root_class))
 @synthesize collision = _collision; // expected-note {{property synthesized here}}
 
 @end
+
+// This used to crash because we'd temporarly store the weak attribute on the
+// declaration specifier, then deallocate it when clearing the declarator.
+id i1, __weak i2, i3;
-- 
2.40.0