Do not warn about format strings that are indexed string literals.

[meikeb]

Summary

The warning for a format string not being a string literal and therefore
being potentially insecure is overly strict for indices into string
literals. This fix checks if the index into the string literal is
precomputable. If that's the case it will check if the suffix of that
string literal is a valid format string string literal. It will still
issue the aforementioned warning for out of range indices into the
string literal.

[srhines]

lib/Sema/SemaChecking.cpp

3856

It might be good to mention that Offset now goes back to the caller to allow for checking of string literal suffixes.

3900

separately

[meikeb]

Fix typo.

[meikeb]

lib/Sema/SemaChecking.cpp

3856

I'm not sure if that should be mentioned here because it is a very high level comment and the suffix of a string literal is a string literal itself.

[rsmith]

Please also handle expressions of the form &"str"[i]. We warn (under -Wstring-plus-int) for "str" + i and suggest rewriting into that form, so we should also handle that form here.

lib/Sema/SemaChecking.cpp

3864

Why is this passed by reference? It's just an input, not an output, right?

4062–4067

This doesn't seem like it preserves enough information for the downstream code to give correct caret diagnostics pointing at locations within the string. It seems like it would be extremely complicated to maintain the necessary invariants to make that work (you'd need to create a fake string literal source buffer so that the StringLiteralParser can reparse it, for whichever of the string literal tokens the offset ends up within). Have you looked at how much work it'd be to feed a starting offset into CheckFormatString instead?

4089–4090

What happens if one of these expressions is value-dependent? The evaluator can crash or assert if given a value-dependent expression. If we don't defer these checks in dependent contexts, you'll need to handle that possibility somehow.

Example:

template<int N> void f(const char *p) {
  printf("blah blah %s" + N, p);
}

4097–4100

This will assert if the result doesn't fit into 64 bits, and it's not guaranteed to (if one of the operands was cast to __int128, for instance). You could use getLimitedValue instead, with some suitable limit.

[meikeb]

lib/Sema/SemaChecking.cpp

4089–4090

I think I don't understand what you are trying to tell me. Especially the example you provided does just fine and behaves as I expected. As far as I followed EvaluateAsInt it does not assert but returns false if we don't get a constexpr here. We warn under -Wformat-nonliteral for value-dependent string literals.

Could you explain this more or provide an example that triggers an assert or explain what behavior is wrong regarding the provided example? Thanks!

[rsmith]

lib/Sema/SemaChecking.cpp

4089–4090

We should not warn for that example, since (for instance) calling f<0> is fine (we should warn for f<11>, though, since it has no format specifiers).

While EvaluateAsInt happens to not assert for that particular value-dependent input, it does assert for some other value-dependent cases. It's not easy for me to find you such a case, because Clang is currently careful to never call this function on a value-dependent expression, but perhaps this will trigger an assert:

struct S { constexpr S(int n) : n(n) {} int n; };
template<int N> void f(const char *p) {
  printf("blah blah %s" + S(N).n, p);
}

[meikeb]

Fix the mentioned issues.

[meikeb]

lib/Sema/SemaChecking.cpp

4089–4090

I hope this additional check fixes this issue. As far as I read the code there were none such asserts in isIntegerConstantExpr(). Thanks for explaining this!

[rsmith]

lib/Sema/SemaChecking.cpp

3842–3843

I can't tell from this declaration what this function is for -- what does "reckon up" mean?

3880

Typo "retruning"

3891–3892

I think you can simplify this to return FExpr->getString().drop_front(Offset);

4073

You should presumably also do this if Offset is >= the length of the string literal (we want printf and friends to at least find the trailing nul byte).

4078–4079

Does this need to be heap-allocated?

4089–4090

OK, I've now checked and the value-dependent case is handled up on line 3953. So just calling EvaluateAsInt here is fine after all.

[meikeb]

Fix mentioned issues.

[meikeb]

Thanks for taking the time and doing these great reviews! Really appreciated!

lib/Sema/SemaChecking.cpp

4073–4074

The = case is part of a different warning. It's checked in CheckFormatString.

[srhines]

My comment is mostly naming considerations to improve clarity. I do have concerns though about the unhandled else path.

lib/Sema/SemaChecking.cpp

3842

Is "computeStringLiteralOffset" or "calculate..." a better name here?

3844

Is "Operand" better than "Addend"? In particular, there is the possibility that we do subtraction of the value instead of addition, so "Addend" makes it a bit confusing. Of course, I then would expect "OperandIsRight" instead of "AddendIsRight" too.

3852

Align -> Canonicalize or Adjust

This is confusing here as "Align" already has too many overloaded meanings in programming (that could be relevant to bitwidths). Canonicalize or Adjust don't have this problem.

3860

Ov -> Overflow

3865

What happens if someone passes something that isn't caught by these two cases? Should we be returning an indicator that the calculation failed? If not, should we assert here?

3867

2 places to fix: "a offset" -> "an offset"

[meikeb]

Fix typos and add assert to sum up offset helper.

[meikeb]

I explained why I chose the names that you commented on. Feel free to add your thoughts if you still think another name would be more fitting.

lib/Sema/SemaChecking.cpp

3842

I thought about that but decided to go with sumUp because compute or calculate sounds like this function would do what we actually do what the caller of this function does (computing the offset). This is just a nice helper to sum up the offset we already have with another piece of offset.

3844

Clang summarizes sub and add as "additive" operands. This is why I think this is fitting. Operand is misleading because it includes a lot more operands than add and sub imo.

3860

I named that in compliance with clang naming. E.g. sadd_ov. It is common in this file to abbreviate variable names with 1-3 characters.

[rsmith]

This basically looks fine to me now. I'm not 100% sold on sumUpStringLiteralOffset being the best name, but I think we have better things to worry about, and it's good enough. Just a couple of minor style issues then I think this is good to go.

lib/Sema/SemaChecking.cpp

3866–3867

Rather than assert(false && XXX);, use either llvm_unreachable(XXX) or change the previous case to be:

else {
  assert(AddendIsRight && BinOpKind == BO_Sub &&
         "operator must be ...");

4078–4079

You can write this more simply as

FormatStringLiteral FStr(StrE, Offset.sextOrTrunc(64).getSExtValue());

[meikeb]

Try to improve offset sum helper function name and fix style issues.

[rsmith]

lib/Sema/SemaChecking.cpp

3864–3867

The suggestion was to remove the condition in the else if and put the assertion inside its body, rather than duplicating it here:

if (BinOpKind == BO_Add)
  // handle add
else {
  assert(it's a subtract);
  // handle sub
}

[meikeb]

Correct assert position in offset sum helper function.

[meikeb]

lib/Sema/SemaChecking.cpp

3864–3867

This is way better. Thank you.

[meikeb]

Thanks for reviewing my patch! It would be great if someone could submit this patch for me.

[srhines]

I will take care of submitting this. Thanks for the reviews everybody!

lib/Sema/SemaChecking.cpp

 
 enum StringLiteralCheckType {
   SLCT_NotALiteral,
   SLCT_UncheckedLiteral,
   SLCT_CheckedLiteral
 };
 } // end anonymous namespace
 
 static void CheckFormatString(Sema &S, const StringLiteral *FExpr,
                               const Expr *OrigFormatExpr,
                               ArrayRef<const Expr *> Args,
                               bool HasVAListArg, unsigned format_idx,
                               unsigned firstDataArg,
                               Sema::FormatStringType Type,
                               bool inFunctionCall,
                               Sema::VariadicCallType CallType,
                               llvm::SmallBitVector &CheckedVarArgs,
                               UncoveredArgHandler &UncoveredArg);
 
 // Determine if an expression is a string literal or constant string.
 // If this function returns false on the arguments to a function expecting a
 // format string, we will usually need to emit a warning.
 // True string literals are then checked by CheckFormatString.
 static StringLiteralCheckType
 checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
                       bool HasVAListArg, unsigned format_idx,
                       unsigned firstDataArg, Sema::FormatStringType Type,
                       Sema::VariadicCallType CallType, bool InFunctionCall,
                       llvm::SmallBitVector &CheckedVarArgs,
-                      UncoveredArgHandler &UncoveredArg) {
+                      UncoveredArgHandler &UncoveredArg,
+                      int64_t &Offset) {
  tryAgain:
   if (E->isTypeDependent() || E->isValueDependent())
     return SLCT_NotALiteral;
 
   E = E->IgnoreParenCasts();
 
   if (E->isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull))
     // Technically -Wformat-nonliteral does not warn about this case.
     // The behavior of printf and friends in this case is implementation
     // dependent.  Ideally if the format string cannot be null then
     // it should have a 'nonnull' attribute in the function prototype.
     return SLCT_UncheckedLiteral;
 
   switch (E->getStmtClass()) {
   case Stmt::BinaryConditionalOperatorClass:
   case Stmt::ConditionalOperatorClass: {
     // The expression is a literal if both sub-expressions were, and it was
     // completely checked only if both sub-expressions were checked.
     const AbstractConditionalOperator *C =
         cast<AbstractConditionalOperator>(E);
 
     // Determine whether it is necessary to check both sub-expressions, for
     // example, because the condition expression is a constant that can be
     // evaluated at compile time.
     bool CheckLeft = true, CheckRight = true;
 
     bool Cond;
     if (C->getCond()->EvaluateAsBooleanCondition(Cond, S.getASTContext())) {
       if (Cond)
         CheckRight = false;
       else
         CheckLeft = false;
     }
 
+    // We need to maintain the offsets for the right and the left hand side
+    // separately to check if every possible indexed expression is a valid
+    // string literal. They might have different offsets for different string
+    // literals in the end.
+    int64_t LOffset = Offset;
     StringLiteralCheckType Left;
     if (!CheckLeft)
       Left = SLCT_UncheckedLiteral;
     else {
       Left = checkFormatStringExpr(S, C->getTrueExpr(), Args,
                                    HasVAListArg, format_idx, firstDataArg,
                                    Type, CallType, InFunctionCall,
-                                   CheckedVarArgs, UncoveredArg);
-      if (Left == SLCT_NotALiteral || !CheckRight)
+                                   CheckedVarArgs, UncoveredArg, LOffset);
+      if (Left == SLCT_NotALiteral || !CheckRight) {
+        Offset = LOffset;
         return Left;
       }
+    }
 
+    int64_t ROffset = Offset;
     StringLiteralCheckType Right =
         checkFormatStringExpr(S, C->getFalseExpr(), Args,
                               HasVAListArg, format_idx, firstDataArg,
                               Type, CallType, InFunctionCall, CheckedVarArgs,
-                              UncoveredArg);
+                              UncoveredArg, ROffset);
 
     return (CheckLeft && Left < Right) ? Left : Right;
   }
 
   case Stmt::ImplicitCastExprClass: {
     E = cast<ImplicitCastExpr>(E)->getSubExpr();
     goto tryAgain;
   }
if (const VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl())) {
           // Look through initializers like const char c[] = { "foo" }
           if (const InitListExpr *InitList = dyn_cast<InitListExpr>(Init)) {
             if (InitList->isStringLiteralInit())
               Init = InitList->getInit(0)->IgnoreParenImpCasts();
           }
           return checkFormatStringExpr(S, Init, Args,
                                        HasVAListArg, format_idx,
                                        firstDataArg, Type, CallType,
-                                       /*InFunctionCall*/false, CheckedVarArgs,
-                                       UncoveredArg);
+                                       /*InFunctionCall*/ false, CheckedVarArgs,
+                                       UncoveredArg, Offset);
         }
       }
 
       // For vprintf* functions (i.e., HasVAListArg==true), we add a
       // special check to see if the format string is a function parameter
       // of the function calling the printf function.  If the function
       // has an attribute indicating it is a printf-like function, then we
       // should suppress warnings concerning non-literals being used in a call
if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl())) {
         if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(ND))
           if (MD->isInstance())
             --ArgIndex;
         const Expr *Arg = CE->getArg(ArgIndex - 1);
 
         return checkFormatStringExpr(S, Arg, Args,
                                      HasVAListArg, format_idx, firstDataArg,
                                      Type, CallType, InFunctionCall,
-                                     CheckedVarArgs, UncoveredArg);
+                                     CheckedVarArgs, UncoveredArg, Offset);
       } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
         unsigned BuiltinID = FD->getBuiltinID();
         if (BuiltinID == Builtin::BI__builtin___CFStringMakeConstantString ||
             BuiltinID == Builtin::BI__builtin___NSStringMakeConstantString) {
           const Expr *Arg = CE->getArg(0);
           return checkFormatStringExpr(S, Arg, Args,
                                        HasVAListArg, format_idx,
                                        firstDataArg, Type, CallType,
                                        InFunctionCall, CheckedVarArgs,
-                                       UncoveredArg);
+                                       UncoveredArg, Offset);
         }
       }
     }
 
     return SLCT_NotALiteral;
   }
   case Stmt::ObjCStringLiteralClass:
   case Stmt::StringLiteralClass: {
     const StringLiteral *StrE = nullptr;
 
     if (const ObjCStringLiteral *ObjCFExpr = dyn_cast<ObjCStringLiteral>(E))
       StrE = ObjCFExpr->getString();
     else
       StrE = cast<StringLiteral>(E);
 
     if (StrE) {
+      if (Offset > 0) {
+        // We create a new string literal based on the computed fixed offset
+        // into the original string literal.
+        StrE = StringLiteral::Create(S.Context,
+                                     StrE->getString().drop_front(Offset),
+                                     StrE->getKind(),
+                                     StrE->isPascal(),
+                                     StrE->getType(),
+                                     StrE->getLocStart());
+      } else if (Offset < 0) {
+        // TODO: It would be better to have an explicit warning for out of
+        // bounds literals.
+        return SLCT_NotALiteral;
+      }
       CheckFormatString(S, StrE, E, Args, HasVAListArg, format_idx,
                         firstDataArg, Type, InFunctionCall, CallType,
                         CheckedVarArgs, UncoveredArg);
       return SLCT_CheckedLiteral;
     }
 
     return SLCT_NotALiteral;
   }
+  case Stmt::BinaryOperatorClass: {
+    llvm::APSInt LResult;
+    llvm::APSInt RResult;
+
+    const BinaryOperator *BinOp = cast<BinaryOperator>(E);
+
+    // A string literal + an int offset is still a string literal.
+    if (BinOp->isAdditiveOp()) {
+      bool LIsInt = BinOp->getLHS()->EvaluateAsInt(LResult, S.Context);
+      bool RIsInt = BinOp->getRHS()->EvaluateAsInt(RResult, S.Context);
+
+      if (LIsInt != RIsInt) {
+        BinaryOperatorKind BinOpKind = BinOp->getOpcode();
+
+        if (BinOpKind == BO_Add) {
+          if (LIsInt) {
+            Offset += LResult.getExtValue();
+            E = BinOp->getRHS();
+          } else {
+            Offset += RResult.getExtValue();
+            E = BinOp->getLHS();
+          }
+          goto tryAgain;
+        } else if (BinOpKind == BO_Sub) {
+          // We can only subtract from a pointer if we expect a literal.
+          if (RIsInt) {
+            Offset -= RResult.getExtValue();
+            E = BinOp->getLHS();
+            goto tryAgain;
+          }
+        }
+      }
+
+      return SLCT_NotALiteral;
+    }
+  }
 
   default:
     return SLCT_NotALiteral;
   }
 }
 
 Sema::FormatStringType Sema::GetFormatStringType(const FormatAttr *Format) {
   return llvm::StringSwitch<FormatStringType>(Format->getType()->getName())
bool Sema::CheckFormatArguments(ArrayRef<const Expr *> Args,
   // the compiler and thereby (2) can practically remove the source of
   // many format string exploits.
 
   // Format string can be either ObjC string (e.g. @"%d") or
   // C string (e.g. "%d")
   // ObjC string uses the same format specifiers as C string, so we can use
   // the same format string checking logic for both ObjC and C strings.
   UncoveredArgHandler UncoveredArg;
+  int64_t Offset = 0;
   StringLiteralCheckType CT =
       checkFormatStringExpr(*this, OrigFormatExpr, Args, HasVAListArg,
                             format_idx, firstDataArg, Type, CallType,
-                            /*IsFunctionCall*/true, CheckedVarArgs,
-                            UncoveredArg);
+                            /*IsFunctionCall*/ true, CheckedVarArgs,
+                            UncoveredArg, Offset);
 
   // Generate a diagnostic where an uncovered argument is detected.
   if (UncoveredArg.hasUncoveredArg()) {
     unsigned ArgIdx = UncoveredArg.getUncoveredArg() + firstDataArg;
     assert(ArgIdx < Args.size() && "ArgIdx outside bounds");
     UncoveredArg.Diagnose(*this, /*IsFunctionCall*/true, Args[ArgIdx]);
   }
 

test/Sema/format-strings.c

 // <rdar://problem/14178260>
 extern void test_format_security_extra_args(const char*, int, ...)
     __attribute__((__format__(__printf__, 1, 3)));
 void test_format_security_pos(char* string) {
   test_format_security_extra_args(string, 5); // expected-warning {{format string is not a string literal (potentially insecure)}}
   // expected-note@-1{{treat the string as an argument to avoid this}}
 }
 #pragma GCC diagnostic warning "-Wformat-nonliteral"
+
+void test_char_pointer_arithmetic(int b) {
+  const char s1[] = "string";
+  const char s2[] = "%s string";
+
+  printf(s1 - 1); // expected-warning {{format string is not a string literal (potentially insecure)}}
+  // expected-note@-1{{treat the string as an argument to avoid this}}
+
+  printf(s1 + 2); // no-warning
+  printf(s2 + 2); // no-warning
+
+  const char s3[] = "%s string";
+  printf((s3 + 2) - 2); // expected-warning{{more '%' conversions than data arguments}}
+  // expected-note@-2{{format string is defined here}}
+  printf(2 + s2);            // no-warning
+  printf(6 + s2 - 2);        // no-warning
+  printf(2 + (b ? s1 : s2)); // no-warning
+
+  const char s5[] = "string %s";
+  printf(2 + (b ? s2 : s5)); // expected-warning{{more '%' conversions than data arguments}}
+  // expected-note@-2{{format string is defined here}}
+  printf(2 + (b ? s2 : s5), "");     // no-warning
+  printf(2 + (b ? s1 : s2 - 2), ""); // no-warning
+
+  const char s6[] = "%s string";
+  printf(2 + (b ? s1 : s6 - 2)); // expected-warning{{more '%' conversions than data arguments}}
+  // expected-note@-2{{format string is defined here}}
+  printf(1 ? s2 + 2 : s2);         // no-warning
+  printf(0 ? s2 : s2 + 2);         // no-warning
+  printf(2 + s2 + 5 * 3 - 16, ""); // expected-warning{{data argument not used}}
+}