Implement BufferOverlap check for sprint/snprintf

[ArnaudBienner]

Summary

I discussed about this with you @dergachev.a a long time ago on the mailing list ("static or dynamic code analysis for undefined behavior in sprintf") and finally took the time to implement something :)

Let me know what you think about this.

[ArnaudBienner]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

2385

Just realized this should be 2 (position 3, but index 2).

Will upload a newer version of the patch with this change, and new tests

[ArnaudBienner]

Fix start index for sprintf ovlerap check + tests

[ArnaudBienner]

Updating D150430: Implement BufferOverlap check for sprint/snprintf

[ArnaudBienner]

Updating D150430: Implement BufferOverlap check for sprint/snprintf

[steakhal]

I like it.

How about an implementation like this:

void CStringChecker::evalSprintfCommon(CheckerContext &C, const CallExpr *CE,
                                       bool IsBounded) const {
  ProgramStateRef State = C.getState();
  DestinationArgExpr Dest = {CE->getArg(0), 0};

  const auto NumParams = CE->getCalleeDecl()->getAsFunction()->getNumParams();
  assert(CE->getNumArgs() >= NumParams);

  auto AllArguments =
      llvm::make_range(CE->getArgs(), CE->getArgs() + CE->getNumArgs());
  auto VariadicArguments = drop_begin(enumerate(AllArguments), NumParams);

  for (auto [ArgIdx, ArgExpr] : VariadicArguments) {
    // We consider only string buffers
    if (QualType PointeeTy = ArgExpr->getType()->getPointeeType();
        PointeeTy.isNull() || !PointeeTy->isAnyCharacterType())
      continue;
    SourceArgExpr Source = {ArgExpr, unsigned(ArgIdx)};

    // Ensure the buffers do not overlap.
    SizeArgExpr SrcExprAsSizeDummy = {Source.Expression, Source.ArgumentIndex};
    State = CheckOverlap(
        C, State,
        (IsBounded ? SizeArgExpr{CE->getArg(1), 1} : SrcExprAsSizeDummy), Dest,
        Source);
    if (!State)
      return;
  }

  C.addTransition(State);
}

• Using ranges to iterate over the variadic arguments. This way it will just work with both builtins and whatnot.
• Adding a transition to preserve the gained knowledge about the relation of the source and destination buffers. In this case, it won't be really effective, but at least we obey this convention.

Maybe add a few test cases where the warning should not be present.
And it is also valuable to have at least one test case asserting the complete message.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

179–180

Let's express that we expect at least 2 arguments at the callsites.

[steakhal]

Here is the migrated version of the mentioned discussion: https://discourse.llvm.org/t/static-or-dynamic-code-analysis-for-undefined-behavior-in-sprintf/59624

[ArnaudBienner]

• Code review comments

[ArnaudBienner]

Thanks for the review :)

I implemented the suggested changes.

Just one question: PointeeTy.isNull(): is this guaranteed to always work even though getType()->isAnyPointerType() == false?

I'm assuming yes since the tests still pass, but wanted to confirm this is the best way to do this check for char* arguments.

[steakhal]

In D150430#4347365, @ArnaudBienner wrote:

Thanks for the review :)

I implemented the suggested changes.

Just one question: PointeeTy.isNull(): is this guaranteed to always work even though getType()->isAnyPointerType() == false?

I'm assuming yes since the tests still pass, but wanted to confirm this is the best way to do this check for char* arguments.

I was also surprised, but check the definition of that function. It will try to cast the nonnull type to a list of types, and return null if didnt match.

[ArnaudBienner]

Giving this a second thought, I feel like the initial check:

if (!ArgExpr->getType()->isAnyPointerType() ||
    !ArgExpr->getType()->getPointeeType()->isAnyCharacterType())

is better than the new one. To me it reads like "expr type is a pointer and it points to character type" which is more understandable IMHO.

If you're worried about the expression being a bit long, I could move type to a temp variable:

if (const QualType type = ArgExpr->getType();
    !type->isAnyPointerType() ||
    !type->getPointeeType()->isAnyCharacterType())

Though I'm not sure this is really more readable.

What do you think?

Any other suggestion/comment about this patch?

[ArnaudBienner]

@steakhal did you get a chance to look at my comment?

I would really love to see this merged upstream if you think this could be a beneficial change :)

[steakhal]

Yea, it still looks okay. Let me know if you don't have commit access. In that case, also let me know what should be used for the commit author.

[steakhal]

Ah, I can see that pre-merge bots failed due to clang-format violations.
Please make all the touched lines as clang-formatted prior to committing anything.

[ArnaudBienner]

• Code review comments: change back the string buffer check + run clang format

[steakhal]

Can you commit this change or should I do it on your behalf?

[ArnaudBienner]

• clang-format fix

[ArnaudBienner]

Thanks @steakhal for the proposal :) but I pushed it myself: https://github.com/llvm/llvm-project/commit/ce97312d109b21acb97d3ea243e214f20bd87cfc

I used to have svn access, and Chris kindly give me write access to the git repository.

[ArnaudBienner]

@steakhal should the release notes page be updated to add this?

I think this could be beneficial for the users to be aware of that change.

If yes, who is responsible for doing that? Developers? (me) or someone else is taking care of reviewing the list of changes since latest releases?

[steakhal]

TBH we don't really have processes there. And its a bit of a mess, regarding CSA. Usually we forgot to update the release notes, and at best we aggregate the changes just prior a release for the release notes. But we haven't done it for the last two releases for sure.

I was thinking of asking you, but given this context we wouldn't gain much. We would still need to go through the diffs anyway. So, let not touch the notes now.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

 //= CStringChecker.cpp - Checks calls to C string functions --------*- C++ -*-//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This defines CStringChecker, which is an assortment of checks on calls
 // to functions in <string.h>.
 //
 //===----------------------------------------------------------------------===//
 
 #include "InterCheckerAPI.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
CallDescriptionMap<FnCheck> Callbacks = {
       {{CDF_MaybeBuiltin, {"strncasecmp"}, 3},
        &CStringChecker::evalStrncasecmp},
       {{CDF_MaybeBuiltin, {"strsep"}, 2}, &CStringChecker::evalStrsep},
       {{CDF_MaybeBuiltin, {"bcopy"}, 3}, &CStringChecker::evalBcopy},
       {{CDF_MaybeBuiltin, {"bcmp"}, 3},
        std::bind(&CStringChecker::evalMemcmp, _1, _2, _3, CK_Regular)},
       {{CDF_MaybeBuiltin, {"bzero"}, 2}, &CStringChecker::evalBzero},
       {{CDF_MaybeBuiltin, {"explicit_bzero"}, 2}, &CStringChecker::evalBzero},
+      {{CDF_MaybeBuiltin, {"sprintf"}, 2}, &CStringChecker::evalSprintf},
+      {{CDF_MaybeBuiltin, {"snprintf"}, 2}, &CStringChecker::evalSnprintf},
   };
 
   // These require a bit of special handling.
   CallDescription StdCopy{{"std", "copy"}, 3},
       StdCopyBackward{{"std", "copy_backward"}, 3};
 
   FnCheck identifyCall(const CallEvent &Call, CheckerContext &C) const;
   void evalMemcpy(CheckerContext &C, const CallExpr *CE, CharKind CK) const;
public:
   void evalStrsep(CheckerContext &C, const CallExpr *CE) const;
 
   void evalStdCopy(CheckerContext &C, const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext &C, const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext &C, const CallExpr *CE) const;
   void evalMemset(CheckerContext &C, const CallExpr *CE) const;
   void evalBzero(CheckerContext &C, const CallExpr *CE) const;
 
+  void evalSprintf(CheckerContext &C, const CallExpr *CE) const;
+  void evalSnprintf(CheckerContext &C, const CallExpr *CE) const;
+  void evalSprintfCommon(CheckerContext &C, const CallExpr *CE, bool IsBounded,
+                         bool IsBuiltin) const;
+
   // Utility methods
   std::pair<ProgramStateRef , ProgramStateRef >
   static assumeZero(CheckerContext &C,
                     ProgramStateRef state, SVal V, QualType Ty);
 
   static ProgramStateRef setCStringLength(ProgramStateRef state,
                                               const MemRegion *MR,
                                               SVal strLength);
if (!State)
     return;
 
   if (!memsetAux(Buffer.Expression, Zero, Size.Expression, C, State))
     return;
 
   C.addTransition(State);
 }
 
+void CStringChecker::evalSprintf(CheckerContext &C, const CallExpr *CE) const {
+  CurrentFunctionDescription = "'sprintf'";
+  bool IsBI = CE->getBuiltinCallee() == Builtin::BI__builtin___sprintf_chk;
+  evalSprintfCommon(C, CE, /* IsBounded */ false, IsBI);
+}
+
+void CStringChecker::evalSnprintf(CheckerContext &C, const CallExpr *CE) const {
+  CurrentFunctionDescription = "'snprintf'";
+  bool IsBI = CE->getBuiltinCallee() == Builtin::BI__builtin___snprintf_chk;
+  evalSprintfCommon(C, CE, /* IsBounded */ true, IsBI);
+}
+
+void CStringChecker::evalSprintfCommon(CheckerContext &C, const CallExpr *CE,
+                                       bool IsBounded, bool IsBuiltin) const {
+  ProgramStateRef State = C.getState();
+  DestinationArgExpr Dest = {CE->getArg(0), 0};
+
+  const auto NumParams = CE->getCalleeDecl()->getAsFunction()->getNumParams();
+  assert(CE->getNumArgs() >= NumParams);
+
+  const auto AllArguments =
+      llvm::make_range(CE->getArgs(), CE->getArgs() + CE->getNumArgs());
+  const auto VariadicArguments = drop_begin(enumerate(AllArguments), NumParams);
+
+  for (const auto &[ArgIdx, ArgExpr] : VariadicArguments) {
+    // We consider only string buffers
+    if (const QualType type = ArgExpr->getType();
+        !type->isAnyPointerType() ||
+        !type->getPointeeType()->isAnyCharacterType())
+      continue;
+    SourceArgExpr Source = {ArgExpr, unsigned(ArgIdx)};
+
+    // Ensure the buffers do not overlap.
+    SizeArgExpr SrcExprAsSizeDummy = {Source.Expression, Source.ArgumentIndex};
+    State = CheckOverlap(
+        C, State,
+        (IsBounded ? SizeArgExpr{CE->getArg(1), 1} : SrcExprAsSizeDummy), Dest,
+        Source);
+    if (!State)
+      return;
+  }
+
+  C.addTransition(State);
+}
+
 //===----------------------------------------------------------------------===//
 // The driver method, and other Checker callbacks.
 //===----------------------------------------------------------------------===//
 
 CStringChecker::FnCheck CStringChecker::identifyCall(const CallEvent &Call,
                                                      CheckerContext &C) const {
   const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
   if (!CE)

clang/test/Analysis/buffer-overlap.c

+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+//
+// RUN: %clang_analyze_cc1 -verify %s -DVARIANT -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=alpha.unix.cstring.BufferOverlap
+
+// This provides us with four possible sprintf() definitions.
+
+#ifdef USE_BUILTINS
+#define BUILTIN(f) __builtin_##f
+#else /* USE_BUILTINS */
+#define BUILTIN(f) f
+#endif /* USE_BUILTINS */
+
+typedef typeof(sizeof(int)) size_t;
+
+#ifdef VARIANT
+
+#define __sprintf_chk BUILTIN(__sprintf_chk)
+#define __snprintf_chk BUILTIN(__snprintf_chk)
+int __sprintf_chk (char * __restrict str, int flag, size_t os,
+        const char * __restrict fmt, ...);
+int __snprintf_chk (char * __restrict str, size_t len, int flag, size_t os,
+        const char * __restrict fmt, ...);
+
+#define sprintf(str, ...) __sprintf_chk(str, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+#define snprintf(str, len, ...) __snprintf_chk(str, len, 0, __builtin_object_size(str, 0), __VA_ARGS__)
+
+#else /* VARIANT */
+
+#define sprintf BUILTIN(sprintf)
+int sprintf(char *restrict buffer, const char *restrict format, ... );
+int snprintf(char *restrict buffer, size_t bufsz,
+             const char *restrict format, ... );
+#endif /* VARIANT */
+
+void test_sprintf1() {
+  char a[4] = {0};
+  sprintf(a, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf2() {
+  char a[4] = {0};
+  sprintf(a, "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf3() {
+  char a[4] = {0};
+  sprintf(a, "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_sprintf4() {
+  char a[4] = {0};
+  sprintf(a, "%d", 42); // no-warning
+}
+
+void test_sprintf5() {
+  char a[4] = {0};
+  char b[4] = {0};
+  sprintf(a, "%s", b); // no-warning
+}
+
+void test_snprintf1() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf2() {
+  char a[4] = {0};
+  snprintf(a+1, sizeof(a)-1, "%d/%s", 1, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf3() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s", a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf4() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%s/%s", a, a); // expected-warning{{Arguments must not be overlapping buffers}}
+}
+
+void test_snprintf5() {
+  char a[4] = {0};
+  snprintf(a, sizeof(a), "%d", 42); // no-warning
+}
+
+void test_snprintf6() {
+  char a[4] = {0};
+  char b[4] = {0};
+  snprintf(a, sizeof(a), "%s", b); // no-warning
+}