[PATCH] D71566: New checks for fortified sprintf

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Thanks for working on this! I think this would be a really useful diagnostic to have.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:311
 }
+class EstimateSizeFormatHandler
+    : public analyze_format_string::FormatStringHandler {
----------------
nit: `namespace {`


================
Comment at: clang/lib/Sema/SemaChecking.cpp:370
   // FIXME: There are some more useful checks we could be doing here:
   //  - Analyze the format string of sprintf to see how much of buffer is used.
   //  - Evaluate strlen of strcpy arguments, use as object size.
----------------
Can you delete this comment now?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:383
   bool IsChkVariant = false;
+  llvm::APSInt UsedSize = llvm::APSInt::get(-1);
   unsigned SizeIndex, ObjectIndex;
----------------
`Optional<APSInt>` might be a better way of representing this?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:388
     return;
+  case Builtin::BI__builtin___sprintf_chk: {
+    if (auto *StrE = dyn_cast<StringLiteral>(
----------------
Can't you do this for `Builtin::sprintf` as well? The diagnostic would still be worth issuing when `_FORTIFY_SOURCE` is disabled.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:392
+      EstimateSizeFormatHandler H(StrE);
+      StringRef StrRef = StrE->getString();
+      const char *Str = StrRef.data();
----------------
Will this assert on: `sprintf(buf, L"foo");`? Not that that makes any sense, but we shouldn't crash.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:399
+      size_t StrLen =
+          std::min(std::max(TypeSize, size_t(1)) - 1, StrRef.size());
+      if (!analyze_format_string::ParsePrintfString(
----------------
I'm not sure why you're using `min` here, can you add a comment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71566/new/

https://reviews.llvm.org/D71566