[PATCH] D71566: New checks for fortified sprintf

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:745-746
+def warn_fortify_source_format_overflow : Warning<
+  "'%0' will always overflow; destination buffer has size %1,"
+  " but format string expands to at least %2">,
+  InGroup<FortifySource>;
----------------
I'm fine with the current wording because it's consistent with the existing diagnostics, but I wish we would quantify the size with units in these diagnostics. (e.g., mention that this is measured in bytes as opposed to anything else).


================
Comment at: clang/lib/Sema/SemaChecking.cpp:405-406
+  bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
+                             const char *startSpecifier,
+                             unsigned specifierLen) override {
+    const analyze_format_string::OptionalAmount &FW = FS.getFieldWidth();
----------------
Naming nits: `StartSpecifier` and `SpecifierLen`. It looks like `startSpecifier` isn't used and the name can probably just be dropped.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:436-437
+    switch (FS.getConversionSpecifier().getKind()) {
+    case analyze_format_string::ConversionSpecifier::pArg: // leading 0x
+      Size += 3;
+      break;
----------------
Why is size incremented by 3 here instead of 2 as above with a leading 0x?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:448
+    }
+    Size -= specifierLen;
+    return true;
----------------
Guard against wraparound?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:489
+    if (auto *Format = dyn_cast<StringLiteral>(FormatExpr)) {
+
+      if (!Format->isAscii() && !Format->isUTF8())
----------------
Spurious whitespace.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:490
+
+      if (!Format->isAscii() && !Format->isUTF8())
+        return;
----------------
We can handle UTF-8 format strings even when they contain non-ASCII characters?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:8237
       H.DoneProcessing();
+
   } else if (Type == Sema::FST_Scanf) {
----------------
Spurious whitespace change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71566