Ensure bounds are not inserted into unwritable files (#689)

Adds checks to ensure that 3C will not attempt to rewrite in unwritable files
to insert array bounds. The test file in this PR contains an example using a
function body in an unwritable file which fails on the current main. After
changes made to support rewriting in functions declarations without
definitions (#691), this will be able to
happen even when the functions are not defined, and so it will becomes a more
prevalent issue.

Author: john-h-kastner

clang/include/clang/3C/CtxSensAVarBounds.h

@@ -92,6 +92,9 @@ class CtxSensitiveBoundsKeyHandler {
                                  std::map<BoundsKey, BoundsKey> &BKMap,
                                  bool IsGlobal);
 
+  void contextualizeCVar(RecordDecl *RD, std::string AccessKey, bool IsGlobal,
+                         ASTContext *C, ProgramInfo &I);
+
   // Get string that represents a context sensitive key for the struct
   // member access ME.
   std::string getCtxStructKey(MemberExpr *ME, ASTContext *C);

clang/lib/3C/AVarBoundsInfo.cpp

@@ -592,18 +592,37 @@ void PotentialBoundsInfo::addPotentialBoundsPOne(
 }
 
 bool AVarBoundsInfo::isValidBoundVariable(clang::Decl *D) {
+  if (D == nullptr)
+    return false;
+
+  // Any pointer declaration in an unwritable file without existing bounds
+  // annotations is not valid. This ensures we do not add bounds onto pointers
+  // where attempting to rewrite the variable to insert the bound would be an
+  // error. If there are existing bounds, no new bound will be inferred, so no
+  // rewriting will be attempted. By leaving existing bounds as valid, these
+  // bounds can be used infer bounds on other (writeable) declarations.
+  // Non-pointer types are also still valid because these will never need bounds
+  // expression, and they need to remain valid so that they can be used by
+  // existing array pointer bounds.
+  auto PSL = PersistentSourceLoc::mkPSL(D, D->getASTContext());
+  if (!canWrite(PSL.getFileName())) {
+    if (auto *DD = dyn_cast<DeclaratorDecl>(D))
+      return !DD->getType()->isPointerType() || DD->hasBoundsExpr();
+    return false;
+  }
+
   // All return and field values are valid bound variables.
-  if (D && (isa<FunctionDecl>(D) || isa<FieldDecl>(D)))
+  if (isa<FunctionDecl>(D) || isa<FieldDecl>(D))
     return true;
 
   // For Parameters, check if they belong to a valid function.
   // Function pointer types are not considered valid functions, so function
-  // pointer parameters are are disqualified as valid bound variables here.
-  if (auto *PD = dyn_cast_or_null<ParmVarDecl>(D))
+  // pointer parameters are disqualified as valid bound variables here.
+  if (auto *PD = dyn_cast<ParmVarDecl>(D))
     return PD->getParentFunctionOrMethod() != nullptr;
 
-  // For VarDecls, check if these are are not dummy and have a name.
-  if (auto *VD = dyn_cast_or_null<VarDecl>(D))
+  // For VarDecls, check if these are not dummy and have a name.
+  if (auto *VD = dyn_cast<VarDecl>(D))
     return !VD->getNameAsString().empty();
 
   return false;
@@ -651,27 +670,25 @@ bool AVarBoundsInfo::tryGetVariable(clang::Decl *D, BoundsKey &R) {
 
 bool AVarBoundsInfo::tryGetVariable(clang::Expr *E, const ASTContext &C,
                                     BoundsKey &Res) {
-  Optional<llvm::APSInt> OptConsVal;
-  bool Ret = false;
   if (E != nullptr) {
     E = E->IgnoreParenCasts();
-    if (E->getType()->isArithmeticType() &&
-        (OptConsVal = E->getIntegerConstantExpr(C))) {
+
+    // Get the BoundsKey for the constant value if the expression is a constant
+    // integer expression.
+    Optional<llvm::APSInt> OptConsVal = E->getIntegerConstantExpr(C);
+    if (E->getType()->isArithmeticType() && OptConsVal.hasValue()) {
       Res = getVarKey(*OptConsVal);
-      Ret = true;
-    } else if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E)) {
-      auto *D = DRE->getDecl();
-      Ret = tryGetVariable(D, Res);
-      if (!Ret) {
-        assert(false && "Invalid declaration found inside bounds expression");
-      }
-    } else if (MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
-      return tryGetVariable(ME->getMemberDecl(), Res);
-    } else {
-      // assert(false && "Variable inside bounds declaration is an expression");
+      return true;
     }
+
+    // For declarations or struct member references, get the bounds key for the
+    // references variables or field.
+    if (auto *DRE = dyn_cast<DeclRefExpr>(E))
+      return tryGetVariable(DRE->getDecl(), Res);
+    if (auto *ME = dyn_cast<MemberExpr>(E))
+      return tryGetVariable(ME->getMemberDecl(), Res);
   }
-  return Ret;
+  return false;
 }
 
 // Merging bounds B with the present bounds of key L at the same priority P

clang/lib/3C/CtxSensAVarBounds.cpp

@@ -170,7 +170,7 @@ bool CtxSensitiveBoundsKeyHandler::tryGetFieldCSKey(
     FieldDecl *FD, CtxStKeyMap *CSK, const std::string &AK, ASTContext *C,
     ProgramInfo &I, BoundsKey &CSKey) {
   bool RetVal = false;
-  if (CSK->find(AK) != CSK->end()) {
+  if (ABI->isValidBoundVariable(FD) && CSK->find(AK) != CSK->end()) {
     CVarOption CV = I.getVariable(FD, C);
     BoundsKey OrigK;
     if (CV.hasValue() && CV.getValue().hasBoundsKey()) {
@@ -225,44 +225,41 @@ void CtxSensitiveBoundsKeyHandler::contextualizeStructRecord(
   }
 }
 
+void CtxSensitiveBoundsKeyHandler::contextualizeCVar(RecordDecl *RD,
+                                                     std::string AccessKey,
+                                                     bool IsGlobal,
+                                                     ASTContext *C,
+                                                     ProgramInfo &I) {
+  std::string FileName = PersistentSourceLoc::mkPSL(RD, *C).getFileName();
+  if (canWrite(FileName)) {
+    // Context sensitive struct key map.
+    CtxStKeyMap *MECSMap = getCtxStKeyMap(IsGlobal);
+    auto &BKeyMap = (*MECSMap)[AccessKey];
+    contextualizeStructRecord(I, C, RD, AccessKey, BKeyMap, IsGlobal);
+  }
+}
+
 void CtxSensitiveBoundsKeyHandler::contextualizeCVar(MemberExpr *ME,
                                                      ASTContext *C,
                                                      ProgramInfo &I) {
   FieldDecl *FD = dyn_cast_or_null<FieldDecl>(ME->getMemberDecl());
-  if (FD != nullptr) {
-    RecordDecl *RD = FD->getParent();
-    // If the base decl is not null.
-    if (RD != nullptr) {
-      // Get structure access key.
-      StructAccessVisitor SKV(C);
-      SKV.TraverseStmt(ME->getBase()->getExprStmt());
-      std::string AK = SKV.getStructAccessKey();
-      // Context sensitive struct key map.
-      CtxStKeyMap *MECSMap = getCtxStKeyMap(SKV.IsGlobal);
-      auto &BKeyMap = (*MECSMap)[AK];
-      contextualizeStructRecord(I, C, RD, AK, BKeyMap, SKV.IsGlobal);
-    }
+  if (RecordDecl *RD = FD != nullptr ? FD->getParent() : nullptr) {
+    // Get structure access key.
+    StructAccessVisitor SKV(C);
+    SKV.TraverseStmt(ME->getBase()->getExprStmt());
+    contextualizeCVar(RD, SKV.getStructAccessKey(), SKV.IsGlobal, C, I);
   }
 }
 
 void CtxSensitiveBoundsKeyHandler::contextualizeCVar(VarDecl *VD, ASTContext *C,
                                                      ProgramInfo &I) {
-  const RecordType *RT = dyn_cast_or_null<RecordType>(
-      VD->getType()->getUnqualifiedDesugaredType());
-  const RecordDecl *RD = nullptr;
-  if (RT != nullptr) {
-    RD = RT->getDecl();
-  }
-  // If the base decl is not null.
-  if (RT != nullptr) {
+  const auto *RT = dyn_cast_or_null<RecordType>(
+    VD->getType()->getUnqualifiedDesugaredType());
+  if (RecordDecl *RD = RT != nullptr ? RT->getDecl() : nullptr) {
     // Get structure access key.
     StructAccessVisitor SKV(C);
     SKV.processVarDecl(VD);
-    // Context sensitive struct key map.
-    CtxStKeyMap *MECSMap = getCtxStKeyMap(SKV.IsGlobal);
-    std::string AK = SKV.getStructAccessKey();
-    auto &BKeyMap = (*MECSMap)[AK];
-    contextualizeStructRecord(I, C, RD, AK, BKeyMap, SKV.IsGlobal);
+    contextualizeCVar(RD, SKV.getStructAccessKey(), SKV.IsGlobal, C, I);
   }
 }
 

clang/test/3C/base_subdir/unwritable_bounds.c

@@ -0,0 +1,74 @@
+// RUN: rm -rf %t*
+// RUN: cd %S
+// RUN: 3c -alltypes -addcr -output-dir=%t.checked/base_subdir %s --
+
+// The functions called from this file would normally have bounds inferred if
+// they were declared in a writable file. The file is not writable, so the new
+// bounds must not be inferred. This possibility of this happening existed
+// before 3C started inferring itypes on undefined functions, but it became a
+// significant issue and was noticed with the introduction of this feature.
+
+#include "../unwritable_bounds.h"
+void test_functions() {
+  {
+    char s[0];
+    unwrite0(s);
+  }
+  {
+    char s[0];
+    unwrite1(s);
+  }
+  {
+    char s[0];
+    unwrite2(s);
+  }
+  {
+    char s[0];
+    unwrite3(s);
+  }
+  {
+    char s[0];
+    unwrite4(s);
+  }
+  {
+    char s[0];
+    unwrite5(s);
+  }
+  {
+    char s[0];
+    unwrite6(s);
+  }
+}
+
+void test_struct() {
+  // There is a code path for variable declarations and a different path for
+  // uses of a variable. The initializer is required to test the declaration
+  // path.
+  struct arr_struct a = {};
+  for (int i = 0; i < a.n; i++)
+    a.arr[i];
+
+  // I don't quite understand why this was a problem, but it caused an
+  // assertion to fail after apply the fix for the first struct test
+  // case.
+  union e {
+    float d
+  };
+  int i = struct_ret()->c;
+  union e f;
+  f.d;
+}
+
+void test_glob() {
+  for (int i = 0; i < 10; i++)
+    glob0[i];
+
+  for (int i = 0; i < 10; i++)
+    glob1[i];
+
+  for (int i = 0; i < 10; i++)
+    glob2[i];
+
+  for (int i = 0; i < 10; i++)
+    glob3[i];
+}

clang/test/3C/unwritable_bounds.h

@@ -0,0 +1,34 @@
+// Used by base_subdir/unwritable_bounds.c .
+
+void unwrite0(void *p : itype(_Array_ptr<void>));
+void unwrite1(char *p);
+void unwrite2(char *p : itype(_Array_ptr<char>));
+void unwrite3(_Array_ptr<char> p);
+
+void unwrite4(char *p) {
+  for (int i = 0; i < 10; i++)
+    p[i];
+}
+void unwrite5(char *p : itype(_Array_ptr<char>)) {
+  for (int i = 0; i < 10; i++)
+    p[i];
+}
+void unwrite6(_Array_ptr<char> p) {
+  for (int i = 0; i < 10; i++)
+    p[i];
+}
+
+struct arr_struct {
+  int *arr;
+  int n;
+};
+
+struct other_struct {
+  char *c;
+};
+struct other_struct *struct_ret();
+
+int *glob0;
+int *glob1 : itype(_Array_ptr<int>);
+int *glob2 : count(10);
+int *glob3 : itype(_Ptr<int>);