[Sema] Fix a use-after-deallocate of a ParsedAttr

[erik.pilkington]

Summary

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

Thanks for taking a look!
Erik

[aaron.ballman]

LGTM aside from some minor nits. Thanks for this!

clang/include/clang/Sema/ParsedAttr.h

896 ↗

(On Diff #193185)

How about Attrs and PA to fit with usual naming conventions?

clang/test/SemaObjC/arc-property-decl-attrs.m

291 ↗

(On Diff #193185)

Can you add a comment here that explains what this test is intending to cover?

include/clang/Sema/ParsedAttr.h

 
 public:
   AttributeFactory();
   ~AttributeFactory();
 };
 
 class AttributePool {
   friend class AttributeFactory;
+  friend class ParsedAttributes;
   AttributeFactory &Factory;
   llvm::TinyPtrVector<ParsedAttr *> Attrs;
 
   void *allocate(size_t size) {
     return Factory.allocate(size);
   }
 
   ParsedAttr *add(ParsedAttr *attr) {
public:
   AttributePool &getPool() const { return pool; }
 
   void takeAllFrom(ParsedAttributes &attrs) {
     addAll(attrs.begin(), attrs.end());
     attrs.clearListOnly();
     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();
   }
 
   /// Add attribute with expression arguments.
   ParsedAttr *addNew(IdentifierInfo *attrName, SourceRange attrRange,
                      IdentifierInfo *scopeName, SourceLocation scopeLoc,

lib/Sema/SemaType.cpp

done:
   // That might actually be the decl spec if we weren't blocked by
   // anything in the declarator.
   if (considerDeclSpec) {
     if (handleObjCPointerTypeAttr(state, attr, declSpecType)) {
       // Splice the attribute into the decl spec.  Prevents the
       // 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;
     }
   }
 
   // Otherwise, if we found an appropriate chunk, splice the attribute
   // into it.
   if (innermost != -1U) {
     moveAttrFromListToList(attr, declarator.getAttributes(),

test/SemaObjC/arc-property-decl-attrs.m

 
 // no error
 @synthesize p = _p;
 @synthesize p2 = _p2;
 
 @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;