[analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

[steakhal]

Summary

The SymbolMetadata handing was slightly flawed.
Given the following example:

void strcat_models_dst_length_even_if_src_dies(char *src) {
  char dst[8] = "1234";
  strcat(dst, src);
  clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{TRUE}}
}

A metadata symbol was assigned to the cstring length map, representing the new length of the dst:
'dst -> meta{src} + 4' where meta{src} represents the cstring length of the region of src.
Unfortunately, this information was immediately removed from the model, since the src become dead after the evaluation of calling strcat.
This results in the removal of all symbolic expressions (eg metadata symbols) that refers to the dead src memory region.

As of now, MetadataSymbols are used only by the CStringChecker, so this fix is tightly coupled to fixing the resulting issues of that checker.
There was already a FIXME in the test suite for this - which is now resolved.

---

There was a lengthy discussion on the mailing list how to resolve this issue.
In that mailing, @NoQ proposed these steps, which I have implemented in this patch:

1. Remove statement, location context, block count from SymbolMetadata's identity. Let it solely depend on the region.
2. Get rid of the metadata-in-use set. From now on SymbolMetadata, like SymbolRegionValue, is live whenever its region is live.
3. When strlen(R) is used for the first time on a region R, produce $meta<size_t R> as the answer, but *do not store* it in the string length map. We don't need to change the state because the state didn't in fact change. On subsequent strlen(R) calls we'll simply return the same symbol (because the map will remain empty), until the string is changed.
4. If the string is mutated, record its length as usual. But if the string is invalidated (or the new length is completely unknown), do not remove the length from the state; instead, set it to SymbolConjured. Only remove it from the map when the region dies.
5. Keep string length symbols alive as long as they're in the map.

[martong]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

198

I think the comments should be updated as well: remove "in use" and refer to the aliveness of the memregion.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2417

Seems like checkDeadSymbols could have a generic implementation. Perhaps in the following form:

class CStringChecker : public Checker<
                                         check::DeadSymbols<CStringLength>

But this should be done in a separate patch.

Maybe it would make sense to have a generic default check::LiveSymbols<GDM0, GDM1, ...> implementation as well. In that implementation we could iterate over the GDM entries and could mark the dependent (sub)symbols live.

(Note, this is a reminder from our verbal discussion with @steakhal.)

clang/test/Analysis/string.c

1544

It would make sense to split this into two. Only one of them should be in the FIXMEs section with the {{UNKNOWN}}. TRUE expectations could be moved out from the FIXMEs section.

[steakhal]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

441

Why do we even need the tag?
Why is it defaulted to nullptr if it asserts later that the tag must not be null.
I'm confused, somebody help me out :D

516–522

Is it true for modeling checkers too?

524–532

I think it should be removed, but for now I left it to draw attention in the review process.

[steakhal]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

198

Definitely, thanks.

clang/test/Analysis/string.c

1544

This patch does not interfere with the test case below.

However, it would be nice to say that calling strlen on non-null-terminated strings is UB.
Thus expecting the clang_analyzer_eval(strlen(str) >= 5); to be TRUE does not make sense much IMO.

[NoQ]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

441

The tag is necessary so that different checkers could attach different metadata to the same region (possibly even of the same type). A null value indeed makes no sense here.

516–522

This comment is much older than the concept of a "modeling checker" and it has never been true. Checkers need to mark things live. It's part of life! (no pun intended) Like, i agree that non-modeling checkers probably don't need to use this method, but that wasn't what the author was trying to say :) And also there's nothing special about SymbolMetadata; any data that the checker helps keep track of may need to be marked live by the checker, so markInUse() is insufficient.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2417

Nope, there can't be a generic implementation. It depends a lot on the data structure tracked by the checker (the checker's technically allowed to track "a map from regions to maps from lists of expressions to sets of SVals", good luck coming up with a generic checkLiveSymbols() for that) and it most likely isn't uniquely determined by the data structure either.

But what we can do is provide half-baked implementations for the common situations such as "a map from regions to symbols" (eg. this checker, smart pointer checker) or "a map from symbols to an enum of states the object associated with the symbol is in" (malloc checker, stream checker). And even better, we could provide half-baked implementations of entire state traits, not just checkDeadSymbols
/checkLiveSymbols().

2429

Ok, this doesn't look correct (looks like it never was). It's liveness of the region that's important to us, not liveness of the symbol. Because it's the liveness of the region that causes the program to be able to access the map entry.

[steakhal]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

441

Thanks.

516–522

Ok, I will rephrase the comment expressing that this should be used only in modeling checkers.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2417

I think we can postprone the experiments with the generic checkDeadSymbols/checkLiveSymbols implementations.
This patch does not aim to solve that.

2429

Let's say we have this:

void foo() {
  char *p = malloc(12);
  // strlen(p); // no-leak if strlen called, leak warning otherwise...
} // expected-warning {{leak}}

If we were marking the region live here, we would miss the leak warning. That warning is only triggered if the region of the p becomes dead. Which will never become dead if we have a cstring length symbol associated to that region.
I came to this conclusion after implementing your suggested edit above (checking regions instead of symbols).

[martong]

But if the string is invalidated (or the new length is completely unknown), do not remove the length from the state; instead, set it to SymbolConjured.

Where exactly do you implement the above?

When strlen(R) is used for the first time on a region R, produce $meta<size_t R> as the answer, but *do not store* it in the string length map.

And this?

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2379

It's just a tiny nit. But, perhaps we could come up with a more meaningful name instead of Entry and Entries. Maybe Length ?

2384–2385

Umm, these two lines are quite disturbing after each other. Seems like first we remove MR then we add MR again, so, this must not be a noop, right? But then what is happening here exactly? Some comments around here in the code would help.

2429

Is it possible to have two string length symbols that are associated to the same region? Could that cause any problem?
E.g. what will happen below? Will we remove MR twice?

char *p = "asdf"
char *q = p + 1;
strlen(p); strlen(q);

[martong]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2384–2385

Ugh, giving it more thought, we just reset the Value associated to MR. Still, would be nice to comment this there.

[steakhal]

In D86445#2260744, @martong wrote:

But if the string is invalidated (or the new length is completely unknown), do not remove the length from the state; instead, set it to SymbolConjured.

Where exactly do you implement the above?

When strlen(R) is used for the first time on a region R, produce $meta<size_t R> as the answer, but *do not store* it in the string length map.

And this?

Hm yes. I missed them. I will extend the current implementation, rethinking the invalidation cases, when to store and what etc.
It will bloat this patch, but indeed - necessary. Thank you!

However, as I planned to clean the CStringChecker a little bit up, it seems reasonable to me to land those patches before I start working on this to save me from some rebasing :|

What do you think about these patches:

• D84316 [analyzer][NFC] Split CStringChecker to modeling and reporting
• D84979 [analyzer][NFC] Refine CStringLength modeling API

If you like that direction, I can rebase this on top of that. That would help me a lot, to know when & how a given metadata symbol would be used, and check if we indeed handle the invalidation, etc. in the right way.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2379

We will see. Thanks.

2384–2385

Exactly. I will do that, ok.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2429

Which will never become dead if we have a cstring length symbol associated to that region.

That's not how it's supposed to work. Symbols don't keep their associated regions alive, only regions keep symbols associated with them alive.

Don't manipulate region liveness manually at all here. Regardless of strlen(), the heap region dies with p which is the last variable to refer to it. Rely on it to garbage-collect the symbol for strlen(p) but don't actively mutate it.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2429

E.g. what will happen below?

These are different regions. Your p is Element{0, "asdf", char}, whereas your q is Element{1, "asdf", char}.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h

public:
   DefinedOrUnknownSVal getConjuredHeapSymbolVal(const Expr *E,
                                                 const LocationContext *LCtx,
                                                 unsigned Count);
 
   DefinedOrUnknownSVal getDerivedRegionValueSymbolVal(
       SymbolRef parentSymbol, const TypedValueRegion *region);
 
   DefinedSVal getMetadataSymbolVal(const void *symbolTag,
-                                   const MemRegion *region,
-                                   const Expr *expr, QualType type,
-                                   const LocationContext *LCtx,
-                                   unsigned count);
+                                   const MemRegion *region, QualType type);
 
   DefinedSVal getMemberPointer(const NamedDecl *ND);
 
   DefinedSVal getFunctionPointer(const FunctionDecl *func);
 
   DefinedSVal getBlockPointer(const BlockDecl *block, CanQualType locTy,
                               const LocationContext *locContext,
                               unsigned blockCount);

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

public:
 
   // Implement isa<T> support.
   static bool classof(const SymExpr *SE) {
     return SE->getKind() == SymbolExtentKind;
   }
 };
 
 /// SymbolMetadata - Represents path-dependent metadata about a specific region.
 ///  Metadata symbols remain live as long as they are marked as in use before
 ///  dead-symbol sweeping AND their associated regions are still alive.
 ///  Intended for use by checkers.
 class SymbolMetadata : public SymbolData {
   const MemRegion* R;
-  const Stmt *S;
   QualType T;
-  const LocationContext *LCtx;
-  unsigned Count;
   const void *Tag;
 
 public:
-  SymbolMetadata(SymbolID sym, const MemRegion* r, const Stmt *s, QualType t,
-                 const LocationContext *LCtx, unsigned count, const void *tag)
-      : SymbolData(SymbolMetadataKind, sym), R(r), S(s), T(t), LCtx(LCtx),
-        Count(count), Tag(tag) {
-      assert(r);
-      assert(s);
-      assert(isValidTypeForSymbol(t));
-      assert(LCtx);
-      assert(tag);
-    }
+  SymbolMetadata(SymbolID sym, const MemRegion *r, QualType t, const void *tag)
+      : SymbolData(SymbolMetadataKind, sym), R(r), T(t), Tag(tag) {
 
   const MemRegion *getRegion() const { return R; }
-  const Stmt *getStmt() const { return S; }
-  const LocationContext *getLocationContext() const { return LCtx; }
-  unsigned getCount() const { return Count; }
   const void *getTag() const { return Tag; }
 
   QualType getType() const override;
 
   void dumpToStream(raw_ostream &os) const override;
 
-  static void Profile(llvm::FoldingSetNodeID& profile, const MemRegion *R,
-                      const Stmt *S, QualType T, const LocationContext *LCtx,
-                      unsigned Count, const void *Tag) {
+  static void Profile(llvm::FoldingSetNodeID &profile, const MemRegion *R,
+                      QualType T, const void *Tag) {
     profile.AddInteger((unsigned) SymbolMetadataKind);
     profile.AddPointer(R);
-    profile.AddPointer(S);
     profile.Add(T);
-    profile.AddPointer(LCtx);
-    profile.AddInteger(Count);
     profile.AddPointer(Tag);
   }
 
   void Profile(llvm::FoldingSetNodeID& profile) override {
-    Profile(profile, R, S, T, LCtx, Count, Tag);
+    Profile(profile, R, T, Tag);
   }
 
   // Implement isa<T> support.
   static bool classof(const SymExpr *SE) {
     return SE->getKind() == SymbolMetadataKind;
   }
 };
 
const SymbolDerived *getDerivedSymbol(SymbolRef parentSymbol,
                                         const TypedValueRegion *R);
 
   const SymbolExtent *getExtentSymbol(const SubRegion *R);
 
   /// Creates a metadata symbol associated with a specific region.
   ///
   /// VisitCount can be used to differentiate regions corresponding to
   /// different loop iterations, thus, making the symbol path-dependent.
-  const SymbolMetadata *getMetadataSymbol(const MemRegion *R, const Stmt *S,
-                                          QualType T,
-                                          const LocationContext *LCtx,
-                                          unsigned VisitCount,
+  const SymbolMetadata *getMetadataSymbol(const MemRegion *R, QualType T,
                                           const void *SymbolTag = nullptr);
 
   const SymbolCast* getCastSymbol(const SymExpr *Operand,
                                   QualType From, QualType To);
 
   const SymIntExpr *getSymIntExpr(const SymExpr *lhs, BinaryOperator::Opcode op,
                                   const llvm::APSInt& rhs, QualType t);
 
   const SymIntExpr *getSymIntExpr(const SymExpr &lhs, BinaryOperator::Opcode op,
enum SymbolStatus {
     HaveMarkedDependents
   };
 
   using SymbolSetTy = llvm::DenseSet<SymbolRef>;
   using SymbolMapTy = llvm::DenseMap<SymbolRef, SymbolStatus>;
   using RegionSetTy = llvm::DenseSet<const MemRegion *>;
 
   SymbolMapTy TheLiving;
-  SymbolSetTy MetadataInUse;
 
   RegionSetTy RegionRoots;
 
   const StackFrameContext *LCtx;
   const Stmt *Loc;
   SymbolManager& SymMgr;
   StoreRef reapedStore;
   llvm::DenseMap<const MemRegion *, unsigned> includedRegionCache;
public:
 
   const LocationContext *getLocationContext() const { return LCtx; }
 
   bool isLive(SymbolRef sym);
   bool isLiveRegion(const MemRegion *region);
   bool isLive(const Stmt *ExprVal, const LocationContext *LCtx) const;
   bool isLive(const VarRegion *VR, bool includeStoreBindings = false) const;
 
   /// Unconditionally marks a symbol as live.
   ///
   /// This should never be
   /// used by checkers, only by the state infrastructure such as the store and
   /// environment. Checkers should instead use metadata symbols and markInUse.
+  /// TODO: update this comment!!!
   void markLive(SymbolRef sym);
 
   /// Marks a symbol as important to a checker.
   ///
   /// For metadata symbols,
   /// this will keep the symbol alive as long as its associated region is also
   /// live. For other symbols, this has no effect; checkers are not permitted
   /// to influence the life of other symbols. This should be used before any
   /// symbol marking has occurred, i.e. in the MarkLiveSymbols callback.
-  void markInUse(SymbolRef sym);
+  // TODO: remove this function!!!
+  // void markInUse(SymbolRef sym);
 
   using region_iterator = RegionSetTy::const_iterator;
 
   region_iterator region_begin() const { return RegionRoots.begin(); }
   region_iterator region_end() const { return RegionRoots.end(); }
 
   /// Returns whether or not a symbol has been confirmed dead.
   ///

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

if (!hypothetical) {
     const SVal *Recorded = state->get<CStringLength>(MR);
     if (Recorded)
       return *Recorded;
   }
 
   // Otherwise, get a new symbol and update the state.
   SValBuilder &svalBuilder = C.getSValBuilder();
   QualType sizeTy = svalBuilder.getContext().getSizeType();
-  SVal strLength = svalBuilder.getMetadataSymbolVal(CStringChecker::getTag(),
-                                                    MR, Ex, sizeTy,
-                                                    C.getLocationContext(),
-                                                    C.blockCount());
+  SVal strLength =
+      svalBuilder.getMetadataSymbolVal(CStringChecker::getTag(), MR, sizeTy);
 
   if (!hypothetical) {
     if (Optional<NonLoc> strLn = strLength.getAs<NonLoc>()) {
       // In case of unbounded calls strlen etc bound the range to SIZE_MAX/4
       BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
       const llvm::APSInt &maxValInt = BVF.getMaxValue(sizeTy);
       llvm::APSInt fourInt = APSIntType(maxValInt).getValue(4);
       const llvm::APSInt *maxLengthInt = BVF.evalAPSInt(BO_Div, maxValInt,
if (Offset.isValid() && !Offset.hasSymbolicOffset() &&
 
     if (StateNullChar && !StateNonNullChar) {
       // If the value of the second argument of 'memset()' is zero, set the
       // string length of destination buffer to 0 directly.
       State = setCStringLength(State, MR,
                                svalBuilder.makeZeroVal(Ctx.getSizeType()));
     } else if (!StateNullChar && StateNonNullChar) {
       SVal NewStrLen = svalBuilder.getMetadataSymbolVal(
-          CStringChecker::getTag(), MR, DstBuffer, Ctx.getSizeType(),
-          C.getLocationContext(), C.blockCount());
+          CStringChecker::getTag(), MR, Ctx.getSizeType());
 
       // If the value of second argument is not zero, then the string length
       // is at least the size argument.
       SVal NewStrLenGESize = svalBuilder.evalBinOp(
           State, BO_GE, NewStrLen, SizeVal, svalBuilder.getConditionType());
 
       State = setCStringLength(
           State->assume(NewStrLenGESize.castAs<DefinedOrUnknownSVal>(), true),
for (ArrayRef<const MemRegion *>::iterator
     while (const SubRegion *SR = dyn_cast<SubRegion>(MR)) {
       MR = SR->getSuperRegion();
       SuperRegions.insert(MR);
     }
   }
 
   CStringLengthTy::Factory &F = state->get_context<CStringLength>();
 
-  // Then loop over the entries in the current state.
-  for (CStringLengthTy::iterator I = Entries.begin(),
-      E = Entries.end(); I != E; ++I) {
-    const MemRegion *MR = I.getKey();
+  // Overwrite the associated cstring length values of the invalidated regions.
+  for (const auto &Entry : Entries) {
+    const MemRegion *MR = Entry.first;
 
     // Is this entry for a super-region of a changed region?
     if (SuperRegions.count(MR)) {
       Entries = F.remove(Entries, MR);
+      Entries = F.add(Entries, MR, UnknownVal());
       continue;
     }
 
     // Is this entry for a sub-region of a changed region?
     const MemRegion *Super = MR;
     while (const SubRegion *SR = dyn_cast<SubRegion>(Super)) {
       Super = SR->getSuperRegion();
       if (Invalidated.count(Super)) {
         Entries = F.remove(Entries, MR);
+        Entries = F.add(Entries, MR, UnknownVal());
         break;
       }
     }
   }
 
   return state->set<CStringLength>(Entries);
 }
 
 void CStringChecker::checkLiveSymbols(ProgramStateRef state,
     SymbolReaper &SR) const {
   // Mark all symbols in our string length map as valid.
-  CStringLengthTy Entries = state->get<CStringLength>();
-
-  for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end();
-      I != E; ++I) {
-    SVal Len = I.getData();
-
-    for (SymExpr::symbol_iterator si = Len.symbol_begin(),
-        se = Len.symbol_end(); si != se; ++si)
-      SR.markInUse(*si);
+  for (const auto &Entry : state->get<CStringLength>()) {
+    SVal AssociatedLength = Entry.second;
+    const auto SymbolRange = llvm::make_range(AssociatedLength.symbol_begin(),
+                                              AssociatedLength.symbol_end());
+    for (SymbolRef SubSym : SymbolRange)
+      if (isa<SymbolData>(SubSym))
+        SR.markLive(SubSym);
   }
 }
 
 void CStringChecker::checkDeadSymbols(SymbolReaper &SR,
     CheckerContext &C) const {
-  ProgramStateRef state = C.getState();
-  CStringLengthTy Entries = state->get<CStringLength>();
+  ProgramStateRef State = C.getState();
+  CStringLengthTy Entries = State->get<CStringLength>();
   if (Entries.isEmpty())
     return;
 
-  CStringLengthTy::Factory &F = state->get_context<CStringLength>();
-  for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end();
-      I != E; ++I) {
-    SVal Len = I.getData();
+  CStringLengthTy::Factory &F = State->get_context<CStringLength>();
+  for (const auto &Entry : Entries) {
+    const MemRegion *MR = Entry.first;
+    SVal Len = Entry.second;
     if (SymbolRef Sym = Len.getAsSymbol()) {
       if (SR.isDead(Sym))
-        Entries = F.remove(Entries, I.getKey());
+        Entries = F.remove(Entries, MR);
     }
   }
 
-  state = state->set<CStringLength>(Entries);
-  C.addTransition(state);
+  State = State->set<CStringLength>(Entries);
+  C.addTransition(State);
 }
 
 void ento::registerCStringModeling(CheckerManager &Mgr) {
   Mgr.registerChecker<CStringChecker>();
 }
 
 bool ento::shouldRegisterCStringModeling(const CheckerManager &mgr) {
   return true;

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

if (T->isNullPtrType())
     return makeZeroVal(T);
 
   SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, T, VisitCount);
   return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
 }
 
 DefinedSVal SValBuilder::getMetadataSymbolVal(const void *symbolTag,
                                               const MemRegion *region,
-                                              const Expr *expr, QualType type,
-                                              const LocationContext *LCtx,
-                                              unsigned count) {
+                                              QualType type) {
   assert(SymbolManager::canSymbolicate(type) && "Invalid metadata symbol type");
 
-  SymbolRef sym =
-      SymMgr.getMetadataSymbol(region, expr, type, LCtx, count, symbolTag);
+  SymbolRef sym = SymMgr.getMetadataSymbol(region, type, symbolTag);
 
   if (Loc::isLocType(type))
     return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
 
   return nonloc::SymbolVal(sym);
 }
 
 DefinedOrUnknownSVal

clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

if (!SD) {
     new (SD) SymbolExtent(SymbolCounter, R);
     DataSet.InsertNode(SD, InsertPos);
     ++SymbolCounter;
   }
 
   return cast<SymbolExtent>(SD);
 }
 
-const SymbolMetadata *
-SymbolManager::getMetadataSymbol(const MemRegion* R, const Stmt *S, QualType T,
-                                 const LocationContext *LCtx,
-                                 unsigned Count, const void *SymbolTag) {
+const SymbolMetadata *SymbolManager::getMetadataSymbol(const MemRegion *R,
+                                                       QualType T,
+                                                       const void *SymbolTag) {
   llvm::FoldingSetNodeID profile;
-  SymbolMetadata::Profile(profile, R, S, T, LCtx, Count, SymbolTag);
+  SymbolMetadata::Profile(profile, R, T, SymbolTag);
   void *InsertPos;
   SymExpr *SD = DataSet.FindNodeOrInsertPos(profile, InsertPos);
   if (!SD) {
     SD = (SymExpr*) BPAlloc.Allocate<SymbolMetadata>();
-    new (SD) SymbolMetadata(SymbolCounter, R, S, T, LCtx, Count, SymbolTag);
+    new (SD) SymbolMetadata(SymbolCounter, R, T, SymbolTag);
     DataSet.InsertNode(SD, InsertPos);
     ++SymbolCounter;
   }
 
   return cast<SymbolMetadata>(SD);
 }
 
 const SymbolCast*
for (auto SR = dyn_cast<SubRegion>(region); SR;
     if (const auto ER = dyn_cast<ElementRegion>(SR)) {
       SVal Idx = ER->getIndex();
       for (auto SI = Idx.symbol_begin(), SE = Idx.symbol_end(); SI != SE; ++SI)
         markLive(*SI);
     }
   }
 }
 
-void SymbolReaper::markInUse(SymbolRef sym) {
-  if (isa<SymbolMetadata>(sym))
-    MetadataInUse.insert(sym);
-}
-
 bool SymbolReaper::isLiveRegion(const MemRegion *MR) {
   // TODO: For now, liveness of a memory region is equivalent to liveness of its
   // base region. In fact we can do a bit better: say, if a particular FieldDecl
   // is not used later in the path, we can diagnose a leak of a value within
   // that field earlier than, say, the variable that contains the field dies.
   MR = MR->getBaseRegion();
 
   if (RegionRoots.count(MR))
case SymExpr::SymbolConjuredKind:
     break;
   case SymExpr::SymbolDerivedKind:
     KnownLive = isLive(cast<SymbolDerived>(sym)->getParentSymbol());
     break;
   case SymExpr::SymbolExtentKind:
     KnownLive = isLiveRegion(cast<SymbolExtent>(sym)->getRegion());
     break;
   case SymExpr::SymbolMetadataKind:
-    KnownLive = MetadataInUse.count(sym) &&
-                isLiveRegion(cast<SymbolMetadata>(sym)->getRegion());
-    if (KnownLive)
-      MetadataInUse.erase(sym);
+    KnownLive = isLiveRegion(cast<SymbolMetadata>(sym)->getRegion());
     break;
   case SymExpr::SymIntExprKind:
     KnownLive = isLive(cast<SymIntExpr>(sym)->getLHS());
     break;
   case SymExpr::IntSymExprKind:
     KnownLive = isLive(cast<IntSymExpr>(sym)->getRHS());
     break;
   case SymExpr::SymSymExprKind:

clang/test/Analysis/bsd-string.c

void f9(int unknown_size, char* unknown_src, char* unknown_dst){
   len = strlcat(buf,"cd", unknown_size);
   clang_analyzer_eval(len==6);// expected-warning{{TRUE}}
   clang_analyzer_eval(strlen(buf)>=4);// expected-warning{{TRUE}}
 
   //dst is unknown
   len = strlcpy(unknown_dst,"abbc",unknown_size);
   clang_analyzer_eval(len==4);// expected-warning{{TRUE}}
   clang_analyzer_eval(strlen(unknown_dst));// expected-warning{{UNKNOWN}}
+}
+
+// Continue the f9 test, but with a fresh 'buf' variable.
+void f9_extra(int unknown_size, char *unknown_src, char *unknown_dst) {
+  char buf[8];
+  size_t len;
 
   //src is unknown
   len = strlcpy(buf,unknown_src, sizeof(buf));
   clang_analyzer_eval(len);// expected-warning{{UNKNOWN}}
   clang_analyzer_eval(strlen(buf));// expected-warning{{UNKNOWN}}
 
   //src, dst is unknown
   len = strlcpy(unknown_dst, unknown_src, unknown_size);

clang/test/Analysis/string.c

void strcat_too_big(char *dst, char *src) {
   // We assume this can never actually happen, so we don't get a warning.
   if (strlen(dst) != (((size_t)0) - 2))
     return;
   if (strlen(src) != 2)
     return;
   strcat(dst, src);
 }
 
+void strcat_models_dst_length_even_if_src_dies(char *src) {
+  char dst[8] = "1234";
+  strcat(dst, src);
+  clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{TRUE}}
+}
 
 //===----------------------------------------------------------------------===
 // strncpy()
 //===----------------------------------------------------------------------===
 
 #ifdef VARIANT
 
 #define __strncpy_chk BUILTIN(__strncpy_chk)

 #endif
   clang_analyzer_eval(privkey[0] == '\0');
 #ifdef SUPPRESS_OUT_OF_BOUND
   // expected-warning@-2 {{UNKNOWN}}
 #endif
   free(privkey);
 }
 
-//===----------------------------------------------------------------------===
-// FIXMEs
-//===----------------------------------------------------------------------===
-
-// The analyzer_eval call below should evaluate to true. We are being too
-// aggressive in marking the (length of) src symbol dead. The length of dst
-// depends on src. This could be explicitly specified in the checker or the
-// logic for handling MetadataSymbol in SymbolManager needs to change.
 void strcat_symbolic_src_length(char *src) {
-  char dst[8] = "1234";
-  strcat(dst, src);
-  clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{UNKNOWN}}
+  char dst[8] = "1234";
+  strcat(dst, src);
+  clang_analyzer_eval(strlen(dst) >= 4); // expected-warning{{TRUE}}
 }
 
-
-// The analyzer_eval call below should evaluate to true. Most likely the same
-// issue as the test above.
 void strncpy_exactly_matching_buffer2(char *y) {
   if (strlen(y) >= 4)
     return;
 
   char x[4];
   strncpy(x, y, 4); // no-warning
 
   // This time, we know that y fits in x anyway.
-  clang_analyzer_eval(strlen(x) <= 3); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(x) <= 3); // expected-warning{{TRUE}}
 }
 
+//===----------------------------------------------------------------------===
+// FIXMEs
+//===----------------------------------------------------------------------===
+
 void memset7_char_array_nonnull() {
   char str[5] = "abcd";
   clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
   memset(str, '0', 5);
   // FIXME: This should be TRUE.
   clang_analyzer_eval(str[0] == '0');    // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(strlen(str) >= 5); // expected-warning{{TRUE}}
 }