Bounds check argument_with_type_tag attribute.

[mattd]

Summary

Fixes: PR28520

Perform a bounds check on a function's argument list, before accessing any index value specified by an 'argument_with_type_tag' attribute.

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

7922–7925

These should be combined into a single diagnostic: %select{type|argument}0 tag index %1 is greater than the number of arguments specified

include/clang/Sema/Sema.h

10458–10459

const Expr * (space before the *). Same below in the definition.

lib/Sema/SemaChecking.cpp

12345

Spurious newline?

12365

Spurious newline?

test/Sema/error-type-safety.cpp

4–5

Is this declaration necessary?

17

Can you also add a test for a boundary case so we can be sure there are no off-by-one errors introduced later?

[mattd]

• Removed the new lines.
• Added white space between the data type and pointer character.
• Updated the test to check the exact bounds, and over-bounds cases
• Combined the diagnostic messages into one via 'select'

[mattd]

test/Sema/error-type-safety.cpp

4–5

Hi Aaron, thanks for the input. Yes, defining the tag is necessary in order to test the argument index. If the type tag is not satisfied, the code path checking argument tag index will never be reached. My update will test the bounds and is a little more comprehensive.

[aaron.ballman]

Thanks, LGTM!

[mattd]

Thanks for the approval Aaron. I do not have commit access, would you mind submitting this on my behalf?

[aaron.ballman]

In D40574#939693, @mattd wrote:

Thanks for the approval Aaron. I do not have commit access, would you mind submitting this on my behalf?

Happy to do so -- I've commit in r319383, thank you for the patch!

include/clang/Basic/DiagnosticSemaKinds.td

 // Type safety attributes
 def err_type_tag_for_datatype_not_ice : Error<
   "'type_tag_for_datatype' attribute requires the initializer to be "
   "an %select{integer|integral}0 constant expression">;
 def err_type_tag_for_datatype_too_large : Error<
   "'type_tag_for_datatype' attribute requires the initializer to be "
   "an %select{integer|integral}0 constant expression "
   "that can be represented by a 64 bit integer">;
+def err_tag_index_out_of_range : Error<
+  "%select{type tag|argument}0 index %1 is greater than the number of arguments specified">;
 def warn_type_tag_for_datatype_wrong_kind : Warning<
   "this type tag was not designed to be used with this function">,
   InGroup<TypeSafety>;
 def warn_type_safety_type_mismatch : Warning<
   "argument type %0 doesn't match specified %1 type tag "
   "%select{that requires %3|}2">, InGroup<TypeSafety>;
 def warn_type_safety_null_pointer_required : Warning<
   "specified %0 type tag requires a null pointer">, InGroup<TypeSafety>;
 
 // Generic selections.

include/clang/Sema/Sema.h

 private:
   /// \brief A map from magic value to type information.
   std::unique_ptr<llvm::DenseMap<TypeTagMagicValue, TypeTagData>>
       TypeTagForDatatypeMagicValues;
 
   /// \brief Peform checks on a call of a function with argument_with_type_tag
   /// or pointer_with_type_tag attributes.
   void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
-                                const Expr * const *ExprArgs);
+                                const ArrayRef<const Expr *> ExprArgs,
+                                SourceLocation CallSiteLoc);
 
   /// \brief Check if we are taking the address of a packed field
   /// as this may be a problem if the pointer value is dereferenced.
   void CheckAddressOfPackedMember(Expr *rhs);
 
   /// \brief The parser's current scope.
   ///
   /// The parser maintains this state here.

lib/Sema/SemaChecking.cpp

void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto,
   }
 
   if (FDecl || Proto) {
     CheckNonNullArguments(*this, FDecl, Proto, Args, Loc);
 
     // Type safety checking.
     if (FDecl) {
       for (const auto *I : FDecl->specific_attrs<ArgumentWithTypeTagAttr>())
-        CheckArgumentWithTypeTag(I, Args.data());
+        CheckArgumentWithTypeTag(I, Args, Loc);
     }
   }
 
   if (FD)
     diagnoseArgDependentDiagnoseIfAttrs(FD, ThisArg, Args, Loc);
 }
 
 /// CheckConstructorCall - Check a constructor call for correctness and safety
static bool IsSameCharType(QualType T1, QualType T2) {
 
   return (T1Kind == BuiltinType::SChar  && T2Kind == BuiltinType::Char_S) ||
          (T1Kind == BuiltinType::UChar  && T2Kind == BuiltinType::Char_U) ||
          (T1Kind == BuiltinType::Char_U && T2Kind == BuiltinType::UChar) ||
          (T1Kind == BuiltinType::Char_S && T2Kind == BuiltinType::SChar);
 }
 
 void Sema::CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
-                                    const Expr * const *ExprArgs) {
+                                    const ArrayRef<const Expr *> ExprArgs,
+                                    SourceLocation CallSiteLoc) {
   const IdentifierInfo *ArgumentKind = Attr->getArgumentKind();
   bool IsPointerAttr = Attr->getIsPointer();
 
+  // Retrieve the argument representing the 'type_tag'.
+  if (Attr->getTypeTagIdx() >= ExprArgs.size()) {
+    // Add 1 to display the user's specified value.
+    Diag(CallSiteLoc, diag::err_tag_index_out_of_range)
+        << 0 << Attr->getTypeTagIdx() + 1;
+    return;
+  }
   const Expr *TypeTagExpr = ExprArgs[Attr->getTypeTagIdx()];
   bool FoundWrongKind;
   TypeTagData TypeInfo;
   if (!GetMatchingCType(ArgumentKind, TypeTagExpr, Context,
                         TypeTagForDatatypeMagicValues.get(),
                         FoundWrongKind, TypeInfo)) {
     if (FoundWrongKind)
       Diag(TypeTagExpr->getExprLoc(),
            diag::warn_type_tag_for_datatype_wrong_kind)
         << TypeTagExpr->getSourceRange();
     return;
   }
 
+  // Retrieve the argument representing the 'arg_idx'.
+  if (Attr->getArgumentIdx() >= ExprArgs.size()) {
+    // Add 1 to display the user's specified value.
+    Diag(CallSiteLoc, diag::err_tag_index_out_of_range)
+        << 1 << Attr->getArgumentIdx() + 1;
+    return;
+  }
   const Expr *ArgumentExpr = ExprArgs[Attr->getArgumentIdx()];
   if (IsPointerAttr) {
     // Skip implicit cast of pointer to `void *' (as a function argument).
     if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(ArgumentExpr))
       if (ICE->getType()->isVoidPointerType() &&
           ICE->getCastKind() == CK_BitCast)
         ArgumentExpr = ICE->getSubExpr();
   }
   QualType ArgumentType = ArgumentExpr->getType();
 

test/Sema/error-type-safety.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define INT_TAG 42
+
+static const int test_in
+  __attribute__((type_tag_for_datatype(test, int))) = INT_TAG;
+
+// Argument index: 1, Type tag index: 2
+void test_bounds_index(...)
+  __attribute__((argument_with_type_tag(test, 1, 2)));
+
+// Argument index: 3, Type tag index: 1
+void test_bounds_arg_index(...)
+  __attribute__((argument_with_type_tag(test, 3, 1)));
+
+void test_bounds()
+{
+  // Test the boundary edges (ensure no off-by-one) with argument indexing.
+  test_bounds_index(1, INT_TAG);
+
+  test_bounds_index(1); // expected-error {{type tag index 2 is greater than the number of arguments specified}}
+  test_bounds_arg_index(INT_TAG, 1); // expected-error {{argument index 3 is greater than the number of arguments specified}}
+}