[clang][BufferUsage] Fix a StringRef lifetime issue

[tbaederr]

The code before assigned the std::string returned from tryEvaluateString() to the StringRef, but it was possible that the underlying data of that string vanished in the meantime, passing invalid stack memory to ParsePrintfString.

Fix this by using two different code paths for the getCharByteWidth() == 1 case and the tryEvaluateString() one.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Timm Baeder (tbaederr)

Changes

The code before assigned the std::string returned from tryEvaluateString() to the StringRef, but it was possible that the underlying data of that string vanished in the meantime, passing invalid stack memory to ParsePrintfString.

Fix this by using two different code paths for the getCharByteWidth() == 1 case and the tryEvaluateString() one.

---

Full diff: https://github.com/llvm/llvm-project/pull/159109.diff

1 Files Affected:

• (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+13-13)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 1d7b8722103aa..ad3d2346d18be 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -900,22 +900,22 @@ static bool hasUnsafeFormatOrSArg(const CallExpr *Call, const Expr *&UnsafeArg,
   const Expr *Fmt = Call->getArg(FmtArgIdx);
 
   if (auto *SL = dyn_cast<clang::StringLiteral>(Fmt->IgnoreParenImpCasts())) {
-    StringRef FmtStr;
+    if (SL->getCharByteWidth() == 1) {
+      StringRef FmtStr = SL->getString();
+      StringFormatStringHandler Handler(Call, FmtArgIdx, UnsafeArg, Ctx);
 
-    if (SL->getCharByteWidth() == 1)
-      FmtStr = SL->getString();
-    else if (auto EvaledFmtStr = SL->tryEvaluateString(Ctx))
-      FmtStr = *EvaledFmtStr;
-    else
-      goto CHECK_UNSAFE_PTR;
-
-    StringFormatStringHandler Handler(Call, FmtArgIdx, UnsafeArg, Ctx);
+      return analyze_format_string::ParsePrintfString(
+          Handler, FmtStr.begin(), FmtStr.end(), Ctx.getLangOpts(),
+          Ctx.getTargetInfo(), isKprintf);
+    }
 
-    return analyze_format_string::ParsePrintfString(
-        Handler, FmtStr.begin(), FmtStr.end(), Ctx.getLangOpts(),
-        Ctx.getTargetInfo(), isKprintf);
+    if (auto FmtStr = SL->tryEvaluateString(Ctx)) {
+      StringFormatStringHandler Handler(Call, FmtArgIdx, UnsafeArg, Ctx);
+      return analyze_format_string::ParsePrintfString(
+          Handler, FmtStr->data(), FmtStr->data() + FmtStr->size(),
+          Ctx.getLangOpts(), Ctx.getTargetInfo(), isKprintf);
+    }
   }
-CHECK_UNSAFE_PTR:
   // If format is not a string literal, we cannot analyze the format string.
   // In this case, this call is considered unsafe if at least one argument
   // (including the format argument) is unsafe pointer.

[ziqingluo-90]

Thanks for the fix!

[shafik]

Just to clarify here, the memory did not just "vanish" it was because it was using the return value of of scope and therefore the lifetime ended and we had undefined behavior at that point.