Skip to content

Commit e217071

Browse files
authored
Fix TString::FormImp buffer safety and va_list handling (#22228)
As reported here: https://github.com/root-project/root/security/code-scanning/1843 - Fixes #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
1 parent 7142ae7 commit e217071

1 file changed

Lines changed: 24 additions & 23 deletions

File tree

core/base/src/TString.cxx

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2315,33 +2315,34 @@ TObjArray *TString::Tokenize(const TString &delim) const
23152315

23162316
void TString::FormImp(const char *fmt, va_list ap)
23172317
{
2318-
Ssiz_t buflen = 20 + 20 * strlen(fmt); // pick a number, any strictly positive number
2319-
buflen = Clobber(buflen); // Update buflen, as Clobber clamps length to MaxSize (if Fatal does not abort)
2318+
va_list ap_len;
2319+
R__VA_COPY(ap_len, ap);
23202320

2321-
va_list sap;
2322-
R__VA_COPY(sap, ap);
2321+
// First pass: determine required size (excluding '\0')
2322+
int n = vsnprintf(nullptr, 0, fmt, ap_len);
2323+
va_end(ap_len);
23232324

2324-
int n, vc = 0;
2325-
again:
2326-
n = vsnprintf(GetPointer(), buflen, fmt, ap);
2327-
// old vsnprintf's return -1 if string is truncated new ones return
2328-
// total number of characters that would have been written
2329-
if (n == -1 || n >= buflen) {
2330-
if (n == -1)
2331-
buflen *= 2;
2332-
else
2333-
buflen = n+1;
2334-
buflen = Clobber(buflen);
2335-
va_end(ap);
2336-
R__VA_COPY(ap, sap);
2337-
vc = 1;
2338-
goto again;
2325+
if (n < 0) {
2326+
// Formatting error
2327+
Clear();
2328+
return;
23392329
}
2340-
va_end(sap);
2341-
if (vc)
2342-
va_end(ap);
23432330

2344-
SetSize(strlen(Data()));
2331+
// Request enough space (including null terminator)
2332+
Ssiz_t needed = Clobber(n + 1);
2333+
2334+
// Safety: Clobber may clamp to MaxSize
2335+
if (needed <= 0 || needed <= n) {
2336+
Clear();
2337+
return;
2338+
}
2339+
2340+
va_list ap_out;
2341+
R__VA_COPY(ap_out, ap);
2342+
vsnprintf(GetPointer(), needed, fmt, ap_out);
2343+
va_end(ap_out);
2344+
2345+
SetSize(n);
23452346
}
23462347

23472348
////////////////////////////////////////////////////////////////////////////////

0 commit comments

Comments
 (0)