From e368faf007ef8d47e5567e9621606cc0e669b5e5 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Mon, 11 Aug 2025 17:54:49 +0200 Subject: [PATCH 01/12] [core] reduce maxsize of string to prevent int overflow when calling Align Fixes a compiler warning often seen in the CI of the PRs --- core/base/inc/TString.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/base/inc/TString.h b/core/base/inc/TString.h index 296e9d8b86938..f301318403d73 100644 --- a/core/base/inc/TString.h +++ b/core/base/inc/TString.h @@ -255,9 +255,9 @@ friend std::strong_ordering operator<=>(const TString &s1, const TString &s2) { char *GetPointer() { return IsLong() ? GetLongPointer() : GetShortPointer(); } const char *GetPointer() const { return IsLong() ? GetLongPointer() : GetShortPointer(); } #ifdef R__BYTESWAP - static Ssiz_t MaxSize() { return kMaxInt - 1; } + static Ssiz_t MaxSize() { return kMaxInt - 1 - (kAlignment - 1); } #else - static Ssiz_t MaxSize() { return (kMaxInt >> 1) - 1; } + static Ssiz_t MaxSize() { return (kMaxInt >> 1) - 1 - (kAlignment - 1); } #endif void UnLink() const { if (IsLong()) delete [] fRep.fLong.fData; } void Zero() { From a0765d8ab3f1d19774a827f413d09baee1ddcb46 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Mon, 11 Aug 2025 18:46:30 +0200 Subject: [PATCH 02/12] [core] amend AdjustCapacity limits and add docu --- core/base/src/TString.cxx | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx index fc67c27f73373..dd9fb9b0234c1 100644 --- a/core/base/src/TString.cxx +++ b/core/base/src/TString.cxx @@ -23,6 +23,14 @@ strings (<15 on 64-bit and <11 on 32-bit) are contained in the TString internal data structure without the need for mallocing the required space. +\note TString can store a maximum of MaxSize()=2147483631 characters. +Trying to allocate larger buffers might throw std::bad_alloc or raise +Fatal errors or lead to undefined behavior. Likewise, there is no safety +check if you pass a Long64_t to the class functions, they will be silently +rounded to Int_t and lead to an integer overflow (negative value). +For future designs, consider using std::string instead, which has a larger +maximum size. + Substring operations are provided by the TSubString class, which holds a reference to the original string and its data, along with the offset and length of the substring. To retrieve the substring @@ -1220,11 +1228,11 @@ void TString::AssertElement(Ssiz_t i) const Ssiz_t TString::AdjustCapacity(Ssiz_t oldCap, Ssiz_t newCap) { Ssiz_t ms = MaxSize(); - if (newCap > ms - 1) { + if (newCap > ms) { Fatal("TString::AdjustCapacity", "capacity too large (%d, max = %d)", newCap, ms); } - Ssiz_t cap = oldCap < ms / 2 - kAlignment ? + Ssiz_t cap = oldCap < ms / 2 ? Recommend(std::max(newCap, 2 * oldCap)) : ms - 1; return cap; } From fa8eac9cff1bf74c408d05639631ae2dfcc43c49 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Tue, 12 Aug 2025 08:49:21 +0200 Subject: [PATCH 03/12] [core] restore behavior as suggested by pcanal --- core/base/inc/TString.h | 4 ++-- core/base/src/TString.cxx | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/base/inc/TString.h b/core/base/inc/TString.h index f301318403d73..296e9d8b86938 100644 --- a/core/base/inc/TString.h +++ b/core/base/inc/TString.h @@ -255,9 +255,9 @@ friend std::strong_ordering operator<=>(const TString &s1, const TString &s2) { char *GetPointer() { return IsLong() ? GetLongPointer() : GetShortPointer(); } const char *GetPointer() const { return IsLong() ? GetLongPointer() : GetShortPointer(); } #ifdef R__BYTESWAP - static Ssiz_t MaxSize() { return kMaxInt - 1 - (kAlignment - 1); } + static Ssiz_t MaxSize() { return kMaxInt - 1; } #else - static Ssiz_t MaxSize() { return (kMaxInt >> 1) - 1 - (kAlignment - 1); } + static Ssiz_t MaxSize() { return (kMaxInt >> 1) - 1; } #endif void UnLink() const { if (IsLong()) delete [] fRep.fLong.fData; } void Zero() { diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx index dd9fb9b0234c1..337c12093db0e 100644 --- a/core/base/src/TString.cxx +++ b/core/base/src/TString.cxx @@ -23,7 +23,7 @@ strings (<15 on 64-bit and <11 on 32-bit) are contained in the TString internal data structure without the need for mallocing the required space. -\note TString can store a maximum of MaxSize()=2147483631 characters. +\note TString can store a maximum of MaxSize()=2147483646 characters. Trying to allocate larger buffers might throw std::bad_alloc or raise Fatal errors or lead to undefined behavior. Likewise, there is no safety check if you pass a Long64_t to the class functions, they will be silently @@ -1232,7 +1232,7 @@ Ssiz_t TString::AdjustCapacity(Ssiz_t oldCap, Ssiz_t newCap) Fatal("TString::AdjustCapacity", "capacity too large (%d, max = %d)", newCap, ms); } - Ssiz_t cap = oldCap < ms / 2 ? + Ssiz_t cap = oldCap < ms / 2 - kAlignment ? Recommend(std::max(newCap, 2 * oldCap)) : ms - 1; return cap; } From b4f2ead396eb9f65f505066a7bb1d75c7c51e30f Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Tue, 12 Aug 2025 08:51:37 +0200 Subject: [PATCH 04/12] [core] update limits --- core/base/src/TString.cxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx index 337c12093db0e..72024eff774be 100644 --- a/core/base/src/TString.cxx +++ b/core/base/src/TString.cxx @@ -1232,8 +1232,8 @@ Ssiz_t TString::AdjustCapacity(Ssiz_t oldCap, Ssiz_t newCap) Fatal("TString::AdjustCapacity", "capacity too large (%d, max = %d)", newCap, ms); } - Ssiz_t cap = oldCap < ms / 2 - kAlignment ? - Recommend(std::max(newCap, 2 * oldCap)) : ms - 1; + Ssiz_t cap = oldCap <= ms / 2 ? + Recommend(std::max(newCap, 2 * oldCap)) : ms; return cap; } From c8ea11f8a4a7c5efc9a78dfab081bd864f4d0c15 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Tue, 12 Aug 2025 08:54:52 +0200 Subject: [PATCH 05/12] [core] make maxsize constexpr --- core/base/inc/TString.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/base/inc/TString.h b/core/base/inc/TString.h index 296e9d8b86938..8a04657899f5f 100644 --- a/core/base/inc/TString.h +++ b/core/base/inc/TString.h @@ -255,9 +255,9 @@ friend std::strong_ordering operator<=>(const TString &s1, const TString &s2) { char *GetPointer() { return IsLong() ? GetLongPointer() : GetShortPointer(); } const char *GetPointer() const { return IsLong() ? GetLongPointer() : GetShortPointer(); } #ifdef R__BYTESWAP - static Ssiz_t MaxSize() { return kMaxInt - 1; } + static constexpr Ssiz_t MaxSize() { return kMaxInt - 1; } #else - static Ssiz_t MaxSize() { return (kMaxInt >> 1) - 1; } + static constexpr Ssiz_t MaxSize() { return (kMaxInt >> 1) - 1; } #endif void UnLink() const { if (IsLong()) delete [] fRep.fLong.fData; } void Zero() { From 6bb36728acaff4c460c078f37d91fe8ec5b94bfc Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Tue, 12 Aug 2025 08:57:27 +0200 Subject: [PATCH 06/12] [core] mention nullchar --- core/base/src/TString.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx index 72024eff774be..edc0d9c724df8 100644 --- a/core/base/src/TString.cxx +++ b/core/base/src/TString.cxx @@ -23,7 +23,7 @@ strings (<15 on 64-bit and <11 on 32-bit) are contained in the TString internal data structure without the need for mallocing the required space. -\note TString can store a maximum of MaxSize()=2147483646 characters. +\note TString can store a maximum of MaxSize()=2147483646 characters + 1 terminating null. Trying to allocate larger buffers might throw std::bad_alloc or raise Fatal errors or lead to undefined behavior. Likewise, there is no safety check if you pass a Long64_t to the class functions, they will be silently From ed522119975c644ef764a1e9f22d3f35e5d33418 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Tue, 12 Aug 2025 09:20:24 +0200 Subject: [PATCH 07/12] [core] fix integer overflow when calling s+15 inside Align Fixes compiler warning --- core/base/inc/TString.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/core/base/inc/TString.h b/core/base/inc/TString.h index 8a04657899f5f..87e0c3736fdd9 100644 --- a/core/base/inc/TString.h +++ b/core/base/inc/TString.h @@ -230,7 +230,15 @@ friend std::strong_ordering operator<=>(const TString &s1, const TString &s2) { enum { kAlignment = 16 }; static Ssiz_t Align(Ssiz_t s) { return (s + (kAlignment-1)) & ~(kAlignment-1); } - static Ssiz_t Recommend(Ssiz_t s) { return (s < kMinCap ? kMinCap : Align(s+1)) - 1; } + // `s` is expected to be <= MaxSize() = (kMaxInt-1) ; `s + 1` includes the terminating nullchar + static Ssiz_t Recommend(Ssiz_t s) { + if (s < kMinCap) + return kMinCap; + else if (s > MaxSize() - (kAlignment - 1)) + return s; + else + return Align(s + 1) - 1; + } static Ssiz_t AdjustCapacity(Ssiz_t oldCap, Ssiz_t newCap); private: From c70e2d9c08643b790dccbccd551c269641867961 Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Tue, 12 Aug 2025 09:27:27 -0500 Subject: [PATCH 08/12] Formatting Update core/base/inc/TString.h --- core/base/inc/TString.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/base/inc/TString.h b/core/base/inc/TString.h index 87e0c3736fdd9..6ec24f7ea0773 100644 --- a/core/base/inc/TString.h +++ b/core/base/inc/TString.h @@ -231,12 +231,13 @@ friend std::strong_ordering operator<=>(const TString &s1, const TString &s2) { enum { kAlignment = 16 }; static Ssiz_t Align(Ssiz_t s) { return (s + (kAlignment-1)) & ~(kAlignment-1); } // `s` is expected to be <= MaxSize() = (kMaxInt-1) ; `s + 1` includes the terminating nullchar - static Ssiz_t Recommend(Ssiz_t s) { + static Ssiz_t Recommend(Ssiz_t s) + { if (s < kMinCap) return kMinCap; else if (s > MaxSize() - (kAlignment - 1)) return s; - else + else return Align(s + 1) - 1; } static Ssiz_t AdjustCapacity(Ssiz_t oldCap, Ssiz_t newCap); From ae6484ed40dfa8b4887fdb10c585beb44c034d03 Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Tue, 12 Aug 2025 09:28:18 -0500 Subject: [PATCH 09/12] Update core/base/inc/TString.h --- core/base/inc/TString.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/base/inc/TString.h b/core/base/inc/TString.h index 6ec24f7ea0773..55f5ad22daf19 100644 --- a/core/base/inc/TString.h +++ b/core/base/inc/TString.h @@ -264,7 +264,7 @@ friend std::strong_ordering operator<=>(const TString &s1, const TString &s2) { char *GetPointer() { return IsLong() ? GetLongPointer() : GetShortPointer(); } const char *GetPointer() const { return IsLong() ? GetLongPointer() : GetShortPointer(); } #ifdef R__BYTESWAP - static constexpr Ssiz_t MaxSize() { return kMaxInt - 1; } + static constexpr Ssiz_t MaxSize() { return kMaxInt - 1; } #else static constexpr Ssiz_t MaxSize() { return (kMaxInt >> 1) - 1; } #endif From e41fc50050ffc9f482dbac9b04ac02ef55c390b8 Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Tue, 12 Aug 2025 09:28:33 -0500 Subject: [PATCH 10/12] Update core/base/inc/TString.h --- core/base/inc/TString.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/base/inc/TString.h b/core/base/inc/TString.h index 55f5ad22daf19..1f44fb047cbf1 100644 --- a/core/base/inc/TString.h +++ b/core/base/inc/TString.h @@ -266,7 +266,7 @@ friend std::strong_ordering operator<=>(const TString &s1, const TString &s2) { #ifdef R__BYTESWAP static constexpr Ssiz_t MaxSize() { return kMaxInt - 1; } #else - static constexpr Ssiz_t MaxSize() { return (kMaxInt >> 1) - 1; } + static constexpr Ssiz_t MaxSize() { return (kMaxInt >> 1) - 1; } #endif void UnLink() const { if (IsLong()) delete [] fRep.fLong.fData; } void Zero() { From 07a9ccf3879ef185e7ee8a001c9cb66c52d48c61 Mon Sep 17 00:00:00 2001 From: Philippe Canal Date: Tue, 12 Aug 2025 09:29:05 -0500 Subject: [PATCH 11/12] Update core/base/src/TString.cxx --- core/base/src/TString.cxx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx index edc0d9c724df8..4820fd339367b 100644 --- a/core/base/src/TString.cxx +++ b/core/base/src/TString.cxx @@ -1232,8 +1232,7 @@ Ssiz_t TString::AdjustCapacity(Ssiz_t oldCap, Ssiz_t newCap) Fatal("TString::AdjustCapacity", "capacity too large (%d, max = %d)", newCap, ms); } - Ssiz_t cap = oldCap <= ms / 2 ? - Recommend(std::max(newCap, 2 * oldCap)) : ms; + Ssiz_t cap = oldCap <= ms / 2 ? Recommend(std::max(newCap, 2 * oldCap)) : ms; return cap; } From 00694cadee9639998b8d16f6b798cef21ee09d87 Mon Sep 17 00:00:00 2001 From: ferdymercury Date: Tue, 12 Aug 2025 16:31:14 +0200 Subject: [PATCH 12/12] [nfc] disambiguate --- core/base/src/TString.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/base/src/TString.cxx b/core/base/src/TString.cxx index 4820fd339367b..3861da1022082 100644 --- a/core/base/src/TString.cxx +++ b/core/base/src/TString.cxx @@ -23,7 +23,7 @@ strings (<15 on 64-bit and <11 on 32-bit) are contained in the TString internal data structure without the need for mallocing the required space. -\note TString can store a maximum of MaxSize()=2147483646 characters + 1 terminating null. +\note TString can store a maximum of MaxSize()=2147483646 characters; ie 2147483647 bytes if you include the terminating null. Trying to allocate larger buffers might throw std::bad_alloc or raise Fatal errors or lead to undefined behavior. Likewise, there is no safety check if you pass a Long64_t to the class functions, they will be silently