Short-term solution for issue 1009. (#1014)

sulekhark · web-flow · commit d1ae89cc346b · 2021-03-29T06:17:37.000-07:00
* Short-term solution for issue 1009.

* Corrected a slip.
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
@@ -5667,7 +5667,8 @@ class Sema final {
   BoundsExpr *MakeMemberBoundsConcrete(Expr *MemberBase, bool IsArrow,
                                        BoundsExpr *Bounds);
   BoundsExpr *ConcretizeFromFunctionTypeWithArgs(BoundsExpr *Bounds, ArrayRef<Expr *> Args,
-                                                 NonModifyingContext ErrorKind);
+                                                 NonModifyingContext ErrorKind,
+                                                 NonModifyingMessage Message);
 
   /// ConvertToFullyCheckedType: convert an expression E to a fully checked type. This
   /// is used to retype declrefs and member exprs in checked scopes with bounds-safe
diff --git a/clang/lib/Sema/SemaBounds.cpp b/clang/lib/Sema/SemaBounds.cpp
@@ -259,14 +259,17 @@ namespace {
     const ArrayRef<Expr *> Arguments;
     llvm::SmallBitVector VisitedArgs;
     Sema::NonModifyingContext ErrorKind;
+    Sema::NonModifyingMessage Message;
     bool ModifyingArg;
   public:
     CheckForModifyingArgs(Sema &SemaRef, ArrayRef<Expr *> Args,
-                          Sema::NonModifyingContext ErrorKind) :
+                          Sema::NonModifyingContext ErrorKind,
+                          Sema::NonModifyingMessage Message) :
       SemaRef(SemaRef),
       Arguments(Args),
       VisitedArgs(Args.size()),
       ErrorKind(ErrorKind),
+      Message(Message),
       ModifyingArg(false) {}
 
     bool FoundModifyingArg() {
@@ -277,8 +280,7 @@ namespace {
       unsigned index = E->getIndex();
       if (index < Arguments.size() && !VisitedArgs[index]) {
         VisitedArgs.set(index);
-        if (!SemaRef.CheckIsNonModifying(Arguments[index], ErrorKind,
-                                         Sema::NonModifyingMessage::NMM_Error)) {
+        if (!SemaRef.CheckIsNonModifying(Arguments[index], ErrorKind, Message)) {
           ModifyingArg = true;
         }
       }
@@ -313,11 +315,11 @@ namespace {
 
 BoundsExpr *Sema::ConcretizeFromFunctionTypeWithArgs(
   BoundsExpr *Bounds, ArrayRef<Expr *> Args,
-  NonModifyingContext ErrorKind) {
+  NonModifyingContext ErrorKind, NonModifyingMessage Message) {
   if (!Bounds || Bounds->isInvalid())
     return Bounds;
 
-  auto CheckArgs = CheckForModifyingArgs(*this, Args, ErrorKind);
+  auto CheckArgs = CheckForModifyingArgs(*this, Args, ErrorKind, Message);
   CheckArgs.TraverseStmt(Bounds);
   if (CheckArgs.FoundModifyingArg())
     return nullptr;
@@ -3279,7 +3281,7 @@ namespace {
     BoundsExpr *CheckCallExpr(CallExpr *E, CheckedScopeSpecifier CSS,
                               CheckingState &State,
                               CHKCBindTemporaryExpr *Binding = nullptr) {
-      BoundsExpr *ResultBounds = CallExprBounds(E, Binding);
+      BoundsExpr *ResultBounds = CallExprBounds(E, Binding, CSS);
 
       QualType CalleeType = E->getCallee()->getType();
       // Extract the pointee type.  The caller type could be a regular pointer
@@ -3367,7 +3369,8 @@ namespace {
           S.ConcretizeFromFunctionTypeWithArgs(
             const_cast<BoundsExpr *>(ParamBounds),
             ArgExprs,
-            Sema::NonModifyingContext::NMC_Function_Parameter);
+            Sema::NonModifyingContext::NMC_Function_Parameter,
+            Sema::NonModifyingMessage::NMM_Error);
 
         if (!SubstParamBounds)
           continue;
@@ -5936,7 +5939,8 @@ namespace {
     // If ResultName is non-null, it is a temporary variable where the result
     // of the call expression is stored immediately upon return from the call.
     BoundsExpr *CallExprBounds(const CallExpr *CE,
-                               CHKCBindTemporaryExpr *ResultName) {
+                               CHKCBindTemporaryExpr *ResultName,
+                               CheckedScopeSpecifier CSS) {
       BoundsExpr *ReturnBounds = nullptr;
       if (CE->getType()->isCheckedPointerPtrType()) {
         if (CE->getType()->isVoidPointerType())
@@ -5963,7 +5967,7 @@ namespace {
         BoundsExpr *FunBounds = FunReturnAnnots.getBoundsExpr();
         InteropTypeExpr *IType =FunReturnAnnots.getInteropTypeExpr();
         // If there is no return bounds and there is an interop type
-        // annotation, use the bounds impied by the interop type
+        // annotation, use the bounds implied by the interop type
         // annotation.
         if (!FunBounds && IType)
           FunBounds = CreateTypeBasedBounds(nullptr, IType->getType(),
@@ -5978,10 +5982,22 @@ namespace {
                               CE->getNumArgs());
 
         // Concretize Call Bounds with argument expressions.
-        // We can only do this if the argument expressions are non-modifying
+        // We can only do this if the argument expressions are non-modifying.
+        // For argument expressions that are modifying, we issue an error
+        // message only in checked scope because the argument expressions may
+        // be re-evaluated during bounds validation.
+        // TODO: The long-term solution is to introduce temporaries for
+        // modifying argument expressions whose corresponding formals are used
+        // in return or parameter bounds expressions.
+        // Equality between the value computed by an argument expression and its
+        // associated temporary would also need to be recorded.
+        Sema::NonModifyingMessage Message =
+          (CSS == CheckedScopeSpecifier::CSS_Unchecked) ?
+          Sema::NonModifyingMessage::NMM_None :
+          Sema::NonModifyingMessage::NMM_Error;
         ReturnBounds =
           S.ConcretizeFromFunctionTypeWithArgs(FunBounds, ArgExprs,
-                            Sema::NonModifyingContext::NMC_Function_Return);
+                       Sema::NonModifyingContext::NMC_Function_Return, Message);
         // If concretization failed, this means we tried to substitute with
         // a non-modifying expression, which is not allowed by the
         // specification.
diff --git a/clang/test/CheckedC/inferred-bounds/calls.c b/clang/test/CheckedC/inferred-bounds/calls.c
@@ -169,7 +169,6 @@ void f10(_Array_ptr<int> a, _Array_ptr<int> b) {
 
 void f11(_Array_ptr<int> a, _Array_ptr<int> b) {
     _Array_ptr<int> c : bounds(a, a+1) = f_bounds(b++, a); // \
-    // expected-error {{expression not allowed in argument for parameter used in function return bounds}} \
     // expected-error {{inferred bounds for 'c' are unknown after initialization}} \
     // expected-note {{(expanded) declared bounds are 'bounds(a, a + 1)'}}
 }
@@ -230,7 +229,6 @@ void f12(int i, int j) {
 
 void f13(int i, int j) {
     _Array_ptr<int> b : count(i) = f_count(j++, i); // \
-    // expected-error {{expression not allowed in argument for parameter used in function return bounds}} \
     // expected-error {{inferred bounds for 'b' are unknown after initialization}} \
     // expected-note {{(expanded) declared bounds are 'bounds(b, b + i)'}}
 }
@@ -285,7 +283,6 @@ void f14(int i, int j) {
 
 void f15(int i, int j) {
     _Array_ptr<int> b : byte_count(i) = f_byte(j++, i); // \
-    // expected-error {{expression not allowed in argument for parameter used in function return bounds}} \
     // expected-error {{inferred bounds for 'b' are unknown after initialization}} \
     // expected-note {{(expanded) declared bounds are 'bounds((_Array_ptr<char>)b, (_Array_ptr<char>)b + i)'}}
 }
@@ -349,7 +346,6 @@ void f20(int* a, int* b) {
 
 void f21(int* a, int* b) {
     _Array_ptr<int> c : bounds(a, a+1) = f_boundsi(b++, a); // \
-    // expected-error {{expression not allowed in argument for parameter used in function return bounds}} \
     // expected-error {{inferred bounds for 'c' are unknown after initialization}} \
     // expected-note {{(expanded) declared bounds are 'bounds(a, a + 1)'}}
 }
@@ -412,7 +408,6 @@ void f22(int i, int j) {
 
 void f23(int i, int j) {
     _Array_ptr<int> b : count(i) = f_counti(j++, i); // \
-    // expected-error {{expression not allowed in argument for parameter used in function return bounds}} \
     // expected-error {{inferred bounds for 'b' are unknown after initialization}} \
     // expected-note {{(expanded) declared bounds are 'bounds(b, b + i)'}}
 }
@@ -469,7 +464,6 @@ void f24(int i, int j) {
 
 void f25(int i, int j) {
     _Array_ptr<int> b : byte_count(i) = f_bytei(j++, i); // \
-    // expected-error {{expression not allowed in argument for parameter used in function return bounds}} \
     // expected-error {{inferred bounds for 'b' are unknown after initialization}} \
     // expected-note {{(expanded) declared bounds are 'bounds((_Array_ptr<char>)b, (_Array_ptr<char>)b + i)'}}
 }
diff --git a/clang/test/CheckedC/static-checking/bounds-side-effects.c b/clang/test/CheckedC/static-checking/bounds-side-effects.c
@@ -227,3 +227,87 @@ void f105(int len, _Array_ptr<int> p : count(len), int i) {
     len += 1, p = alloc(len * sizeof(int));
 }
 
+extern int memcmp_test(const void *src1 : byte_count(n),
+                       const void *src2 :byte_count(n), unsigned int n);
+extern void *memchr_test(const void *s : byte_count(n), int c, unsigned int n) :
+            bounds(s, (_Array_ptr<char>) s + n);
+extern _Itype_for_any(T) void *malloc_test(unsigned int size) :
+                              itype(_Array_ptr<T>) byte_count(size);
+
+// Checked pointers in checked scope
+void f106()
+_Checked
+{
+  int len = 4;
+  _Nt_array_ptr<char> p : count(5) = "hello";
+  int i = memcmp_test(p, "hello", ++len); // expected-error {{increment expression not allowed in argument for parameter used in function parameter bounds expression}} \
+                                          // expected-error {{increment expression not allowed in argument for parameter used in function parameter bounds expression}}
+}
+
+// Checked pointers in unchecked scope
+void f106_u1()
+{
+  int len = 4;
+  _Nt_array_ptr<char> p : count(5) = "hello";
+  int i = memcmp_test(p, "hello", ++len); // expected-error {{increment expression not allowed in argument for parameter used in function parameter bounds expression}}
+}
+
+// Unchecked pointers in unchecked scope
+void f106_u2()
+{
+  int len = 4;
+  char *p  = "hello";
+  int i = memcmp_test(p, "hello", ++len);
+}
+
+// Checked pointers in checked scope
+void f107()
+_Checked
+{
+  int len = 4;
+  int p _Checked [5] = {'h', 'e', 'l', 'l', 'o'};
+  _Array_ptr<int> pos = memchr_test(p, 'l', ++len); // expected-error {{increment expression not allowed in argument for parameter used in function return bounds expression}} \
+                                                    // expected-error {{increment expression not allowed in argument for parameter used in function parameter bounds expression}}
+}
+
+// Checked pointers in unchecked scope
+void f107_u1()
+{
+  int len = 4;
+  int p _Checked [5] = {'h', 'e', 'l', 'l', 'o'};
+  // Ideally, there should be an error for modifying expressions
+  // used in the return bounds expression also.
+  _Array_ptr<int> pos = memchr_test(p, 'l', ++len); // expected-error {{increment expression not allowed in argument for parameter used in function parameter bounds expression}}
+}
+
+// Unchecked pointers in unchecked scope
+void f107_u2()
+{
+  int len = 4;
+  int p[5] = {'h', 'e', 'l', 'l', 'o'};
+  int *pos = memchr_test(p, 'l', ++len);
+}
+
+// Checked pointers in checked scope
+void f108()
+_Checked
+{
+  int len = 5;
+  _Array_ptr<char> p = malloc_test<char>(++len); // expected-error {{increment expression not allowed in argument for parameter used in function return bounds expression}}
+}
+
+// Checked pointers in unchecked scope
+void f108_u1()
+{
+  int len = 5;
+  // Ideally, there should be an error for modifying expressions
+  // used in the return bounds expression also.
+  _Array_ptr<char> p = malloc_test<char>(++len);
+}
+
+// Unchecked pointers in unchecked scope
+void f108_u2()
+{
+  int len = 5;
+  char *p = malloc_test(++len);
+}
