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

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

lib/Sema/SemaChecking.cpp

12345

Spurious newline?

12366

Spurious newline?

test/Sema/error-type-safety.cpp

3–4

Is this declaration necessary?

16

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

3–4

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_type_tag_out_of_range : Error<
+  "type tag index %0 is greater than the number of arguments specified">;
+def err_arg_tag_out_of_range : Error<
+  "argument tag index %0 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>;

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_type_tag_out_of_range)
+        << 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_arg_tag_out_of_range)
+        << 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
+
+static const int test_void
+  __attribute__((type_tag_for_datatype(test, void))) = 0;
+
+void test_invalid_type_tag_index(int datatype, ...)
+  __attribute__((argument_with_type_tag(test,1,99)));
+
+void test_invalid_arg_index(int datatype, ...)
+  __attribute__((argument_with_type_tag(test,99,1)));
+
+void test_bounds()
+{
+  test_invalid_type_tag_index(0); // expected-error {{type tag index 99 is greater than the number of arguments specified}}
+  test_invalid_arg_index(0); // expected-error {{argument tag index 99 is greater than the number of arguments specified}}
+}