From 413931c426db6658f32277b6a18df43aeb81bbf2 Mon Sep 17 00:00:00 2001
From: Bertrand Bellenot <bellenot@users.noreply.github.com>
Date: Wed, 13 May 2026 09:12:23 +0200
Subject: [PATCH] Fix TString::FormImp buffer safety and va_list handling
 (#22228)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As reported here: https://github.com/root-project/root/security/code-scanning/1843
- Fixes https://github.com/root-project/root/issues/22218
- Fixes https://github.com/root-project/root/security/code-scanning/1843
TString::FormImp used a heuristic buffer size and passed an assumed
length to vsnprintf, which static analyzers could not prove matched
the actual allocated buffer. In addition, the same va_list was reused
across multiple vsnprintf calls, resulting in undefined behavior on
some platforms.
The implementation was rewritten to use a two‑pass vsnprintf approach:
the first pass computes the exact required length, and Clobber() is
used to allocate sufficient space including the null terminator.
A second pass formats the string into the allocated buffer using a
fresh va_list copy.
This change:
- Guarantees that the size passed to vsnprintf matches the allocated buffer
- Eliminates undefined behavior from va_list reuse
- Removes heuristic resizing loops
- Silences static analysis warnings for legitimate reasons
- Preserves existing TString semantics and limits

(cherry picked from commit e21707199e9c841b1883c1032777687968ba8f89)
---
 core/base/src/TString.cxx | 47 ++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx
index 0224eb6461d8e..1dfde2b7ac5b0 100644
--- a/core/base/src/TString.cxx
+++ b/core/base/src/TString.cxx
@@ -2315,33 +2315,34 @@ TObjArray *TString::Tokenize(const TString &delim) const
 
 void TString::FormImp(const char *fmt, va_list ap)
 {
-   Ssiz_t buflen = 20 + 20 * strlen(fmt);    // pick a number, any strictly positive number
-   buflen = Clobber(buflen); // Update buflen, as Clobber clamps length to MaxSize (if Fatal does not abort)
+   va_list ap_len;
+   R__VA_COPY(ap_len, ap);
 
-   va_list sap;
-   R__VA_COPY(sap, ap);
+   // First pass: determine required size (excluding '\0')
+   int n = vsnprintf(nullptr, 0, fmt, ap_len);
+   va_end(ap_len);
 
-   int n, vc = 0;
-again:
-   n = vsnprintf(GetPointer(), buflen, fmt, ap);
-   // old vsnprintf's return -1 if string is truncated new ones return
-   // total number of characters that would have been written
-   if (n == -1 || n >= buflen) {
-      if (n == -1)
-         buflen *= 2;
-      else
-         buflen = n+1;
-      buflen = Clobber(buflen);
-      va_end(ap);
-      R__VA_COPY(ap, sap);
-      vc = 1;
-      goto again;
+   if (n < 0) {
+      // Formatting error
+      Clear();
+      return;
    }
-   va_end(sap);
-   if (vc)
-      va_end(ap);
 
-   SetSize(strlen(Data()));
+   // Request enough space (including null terminator)
+   Ssiz_t needed = Clobber(n + 1);
+
+   // Safety: Clobber may clamp to MaxSize
+   if (needed <= 0 || needed <= n) {
+      Clear();
+      return;
+   }
+
+   va_list ap_out;
+   R__VA_COPY(ap_out, ap);
+   vsnprintf(GetPointer(), needed, fmt, ap_out);
+   va_end(ap_out);
+
+   SetSize(n);
 }
 
 ////////////////////////////////////////////////////////////////////////////////