[scudo] Clean up string handling #86364
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Do not abort if a vector cannot increase its own capacity. In that case, push_back calls silently fail. Modify the ScopedString implementation so that it no longer requires two passes to do the format. Move the helper functions to be private member functions so that they can use push_back directly. This allows the capacity to be increased under the hood and/or silently discards data if the capacity is exceeded and cannot be increased. Add new tests for the Vector and ScopedString for capacity increase failures. Doing this so that if a map call fails, and we are attempting to write an error string, we can still get some of the message dumped. This also avoids crashing in Scudo code, and makes the caller handle any failures.
llvmbot
added
compiler-rt
compiler-rt:scudo
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Christopher Ferris (cferris1000) ChangesDo not abort if a vector cannot increase its own capacity. In that case, push_back calls silently fail. Modify the ScopedString implementation so that it no longer requires two passes to do the format. Move the helper functions to be private member functions so that they can use push_back directly. This allows the capacity to be increased under the hood and/or silently discards data if the capacity is exceeded and cannot be increased. Add new tests for the Vector and ScopedString for capacity increase failures. Doing this so that if a map call fails, and we are attempting to write an error string, we can still get some of the message dumped. This also avoids crashing in Scudo code, and makes the caller handle any failures. Full diff: https://github.com/llvm/llvm-project/pull/86364.diff 8 Files Affected:
diff --git a/compiler-rt/lib/scudo/standalone/fuchsia.cpp b/compiler-rt/lib/scudo/standalone/fuchsia.cpp
index 0788c4198e53ce..2144f1b63f8940 100644
--- a/compiler-rt/lib/scudo/standalone/fuchsia.cpp
+++ b/compiler-rt/lib/scudo/standalone/fuchsia.cpp
@@ -34,11 +34,10 @@ static_assert(ZX_HANDLE_INVALID == 0, "");
static void NORETURN dieOnError(zx_status_t Status, const char *FnName,
uptr Size) {
- char Error[128];
- formatString(Error, sizeof(Error),
- "SCUDO ERROR: %s failed with size %zuKB (%s)", FnName,
+ ScopedString Error;
+ Error.append("SCUDO ERROR: %s failed with size %zuKB (%s)", FnName,
Size >> 10, zx_status_get_string(Status));
- outputRaw(Error);
+ outputRaw(Error.data());
die();
}
diff --git a/compiler-rt/lib/scudo/standalone/mem_map_fuchsia.cpp b/compiler-rt/lib/scudo/standalone/mem_map_fuchsia.cpp
index 0566ab0655263e..28e5a11a37f257 100644
--- a/compiler-rt/lib/scudo/standalone/mem_map_fuchsia.cpp
+++ b/compiler-rt/lib/scudo/standalone/mem_map_fuchsia.cpp
@@ -22,11 +22,10 @@ namespace scudo {
static void NORETURN dieOnError(zx_status_t Status, const char *FnName,
uptr Size) {
- char Error[128];
- formatString(Error, sizeof(Error),
- "SCUDO ERROR: %s failed with size %zuKB (%s)", FnName,
+ ScopedString Error;
+ Error.append("SCUDO ERROR: %s failed with size %zuKB (%s)", FnName,
Size >> 10, _zx_status_get_string(Status));
- outputRaw(Error);
+ outputRaw(Error.data());
die();
}
diff --git a/compiler-rt/lib/scudo/standalone/report_linux.cpp b/compiler-rt/lib/scudo/standalone/report_linux.cpp
index 6a983036e6cd9f..4e28861c3b3dd9 100644
--- a/compiler-rt/lib/scudo/standalone/report_linux.cpp
+++ b/compiler-rt/lib/scudo/standalone/report_linux.cpp
@@ -24,33 +24,29 @@ namespace scudo {
// Fatal internal map() error (potentially OOM related).
void NORETURN reportMapError(uptr SizeIfOOM) {
- char Error[128] = "Scudo ERROR: internal map failure\n";
+ ScopedString Error;
+ Error.append("Scudo ERROR: internal map failure");
if (SizeIfOOM) {
- formatString(
- Error, sizeof(Error),
- "Scudo ERROR: internal map failure (NO MEMORY) requesting %zuKB\n",
- SizeIfOOM >> 10);
+ Error.append(" (NO MEMORY) requesting %zuKB", SizeIfOOM >> 10);
}
- reportRawError(Error);
+ Error.append("\n");
+ reportRawError(Error.data());
}
void NORETURN reportUnmapError(uptr Addr, uptr Size) {
- char Error[128];
- formatString(Error, sizeof(Error),
- "Scudo ERROR: internal unmap failure (error desc=%s) Addr 0x%zx "
+ ScopedString Error;
+ Error.append("Scudo ERROR: internal unmap failure (error desc=%s) Addr 0x%zx "
"Size %zu\n",
strerror(errno), Addr, Size);
- reportRawError(Error);
+ reportRawError(Error.data());
}
void NORETURN reportProtectError(uptr Addr, uptr Size, int Prot) {
- char Error[128];
- formatString(
- Error, sizeof(Error),
- "Scudo ERROR: internal protect failure (error desc=%s) Addr 0x%zx "
+ ScopedString Error;
+ Error.append("Scudo ERROR: internal protect failure (error desc=%s) Addr 0x%zx "
"Size %zu Prot %x\n",
strerror(errno), Addr, Size, Prot);
- reportRawError(Error);
+ reportRawError(Error.data());
}
} // namespace scudo
diff --git a/compiler-rt/lib/scudo/standalone/string_utils.cpp b/compiler-rt/lib/scudo/standalone/string_utils.cpp
index d4e4e3becd0e1a..3d8121275d496a 100644
--- a/compiler-rt/lib/scudo/standalone/string_utils.cpp
+++ b/compiler-rt/lib/scudo/standalone/string_utils.cpp
@@ -14,18 +14,10 @@
namespace scudo {
-static int appendChar(char **Buffer, const char *BufferEnd, char C) {
- if (*Buffer < BufferEnd) {
- **Buffer = C;
- (*Buffer)++;
- }
- return 1;
-}
-
// Appends number in a given Base to buffer. If its length is less than
// |MinNumberLength|, it is padded with leading zeroes or spaces, depending
// on the value of |PadWithZero|.
-static int appendNumber(char **Buffer, const char *BufferEnd, u64 AbsoluteValue,
+void ScopedString::appendNumber(u64 AbsoluteValue,
u8 Base, u8 MinNumberLength, bool PadWithZero,
bool Negative, bool Upper) {
constexpr uptr MaxLen = 30;
@@ -33,11 +25,11 @@ static int appendNumber(char **Buffer, const char *BufferEnd, u64 AbsoluteValue,
RAW_CHECK(Base == 10 || !Negative);
RAW_CHECK(AbsoluteValue || !Negative);
RAW_CHECK(MinNumberLength < MaxLen);
- int Res = 0;
if (Negative && MinNumberLength)
--MinNumberLength;
- if (Negative && PadWithZero)
- Res += appendChar(Buffer, BufferEnd, '-');
+ if (Negative && PadWithZero) {
+ String.push_back('-');
+ }
uptr NumBuffer[MaxLen];
int Pos = 0;
do {
@@ -55,79 +47,81 @@ static int appendNumber(char **Buffer, const char *BufferEnd, u64 AbsoluteValue,
Pos--;
for (; Pos >= 0 && NumBuffer[Pos] == 0; Pos--) {
char c = (PadWithZero || Pos == 0) ? '0' : ' ';
- Res += appendChar(Buffer, BufferEnd, c);
+ String.push_back(c);
}
if (Negative && !PadWithZero)
- Res += appendChar(Buffer, BufferEnd, '-');
+ String.push_back('-');
for (; Pos >= 0; Pos--) {
char Digit = static_cast<char>(NumBuffer[Pos]);
Digit = static_cast<char>((Digit < 10) ? '0' + Digit
: (Upper ? 'A' : 'a') + Digit - 10);
- Res += appendChar(Buffer, BufferEnd, Digit);
+ String.push_back(Digit);
}
- return Res;
}
-static int appendUnsigned(char **Buffer, const char *BufferEnd, u64 Num,
+void ScopedString::appendUnsigned(u64 Num,
u8 Base, u8 MinNumberLength, bool PadWithZero,
bool Upper) {
- return appendNumber(Buffer, BufferEnd, Num, Base, MinNumberLength,
+ appendNumber(Num, Base, MinNumberLength,
PadWithZero, /*Negative=*/false, Upper);
}
-static int appendSignedDecimal(char **Buffer, const char *BufferEnd, s64 Num,
+void ScopedString::appendSignedDecimal(s64 Num,
u8 MinNumberLength, bool PadWithZero) {
const bool Negative = (Num < 0);
const u64 UnsignedNum = (Num == INT64_MIN)
? static_cast<u64>(INT64_MAX) + 1
: static_cast<u64>(Negative ? -Num : Num);
- return appendNumber(Buffer, BufferEnd, UnsignedNum, 10, MinNumberLength,
+ appendNumber(UnsignedNum, 10, MinNumberLength,
PadWithZero, Negative, /*Upper=*/false);
}
+#include <stdio.h>
+
// Use the fact that explicitly requesting 0 Width (%0s) results in UB and
// interpret Width == 0 as "no Width requested":
// Width == 0 - no Width requested
// Width < 0 - left-justify S within and pad it to -Width chars, if necessary
// Width > 0 - right-justify S, not implemented yet
-static int appendString(char **Buffer, const char *BufferEnd, int Width,
- int MaxChars, const char *S) {
+void ScopedString::appendString(int Width, int MaxChars, const char *S) {
if (!S)
S = "<null>";
- int Res = 0;
+ int NumChars = 0;
for (; *S; S++) {
- if (MaxChars >= 0 && Res >= MaxChars)
+ if (MaxChars >= 0 && NumChars >= MaxChars)
break;
- Res += appendChar(Buffer, BufferEnd, *S);
+ String.push_back(*S);
+ NumChars++;
+ }
+ if (Width < 0) {
+ // Only left justification supported.
+ Width = -Width - NumChars;
+ while (Width-- > 0)
+ String.push_back(' ');
}
- // Only the left justified strings are supported.
- while (Width < -Res)
- Res += appendChar(Buffer, BufferEnd, ' ');
- return Res;
}
-static int appendPointer(char **Buffer, const char *BufferEnd, u64 ptr_value) {
- int Res = 0;
- Res += appendString(Buffer, BufferEnd, 0, -1, "0x");
- Res += appendUnsigned(Buffer, BufferEnd, ptr_value, 16,
- SCUDO_POINTER_FORMAT_LENGTH, /*PadWithZero=*/true,
- /*Upper=*/false);
- return Res;
+void ScopedString::appendPointer(u64 ptr_value) {
+ appendString(0, -1, "0x");
+ appendUnsigned(ptr_value, 16,
+ SCUDO_POINTER_FORMAT_LENGTH, /*PadWithZero=*/true,
+ /*Upper=*/false);
}
-static int formatString(char *Buffer, uptr BufferLength, const char *Format,
- va_list Args) {
+void ScopedString::vappend(const char *Format, va_list &Args) {
+ // Since the string contains the '\0' terminator, put our size before it
+ // so that push_back calls work correctly.
+ DCHECK(String.size() > 0);
+ String.resize(String.size() - 1);
+
static const char *PrintfFormatsHelp =
- "Supported formatString formats: %([0-9]*)?(z|ll)?{d,u,x,X}; %p; "
+ "Supported formats: %([0-9]*)?(z|ll)?{d,u,x,X}; %p; "
"%[-]([0-9]*)?(\\.\\*)?s; %c\n";
RAW_CHECK(Format);
- RAW_CHECK(BufferLength > 0);
- const char *BufferEnd = &Buffer[BufferLength - 1];
const char *Cur = Format;
- int Res = 0;
for (; *Cur; Cur++) {
if (*Cur != '%') {
- Res += appendChar(&Buffer, BufferEnd, *Cur);
+ String.push_back(*Cur);
continue;
}
Cur++;
@@ -162,7 +156,7 @@ static int formatString(char *Buffer, uptr BufferLength, const char *Format,
DVal = HaveLL ? va_arg(Args, s64)
: HaveZ ? va_arg(Args, sptr)
: va_arg(Args, int);
- Res += appendSignedDecimal(&Buffer, BufferEnd, DVal, Width, PadWithZero);
+ appendSignedDecimal(DVal, Width, PadWithZero);
break;
}
case 'u':
@@ -172,27 +166,25 @@ static int formatString(char *Buffer, uptr BufferLength, const char *Format,
: HaveZ ? va_arg(Args, uptr)
: va_arg(Args, unsigned);
const bool Upper = (*Cur == 'X');
- Res += appendUnsigned(&Buffer, BufferEnd, UVal, (*Cur == 'u') ? 10 : 16,
+ appendUnsigned(UVal, (*Cur == 'u') ? 10 : 16,
Width, PadWithZero, Upper);
break;
}
case 'p': {
RAW_CHECK_MSG(!HaveFlags, PrintfFormatsHelp);
- Res += appendPointer(&Buffer, BufferEnd, va_arg(Args, uptr));
+ appendPointer(va_arg(Args, uptr));
break;
}
case 's': {
RAW_CHECK_MSG(!HaveLength, PrintfFormatsHelp);
// Only left-justified Width is supported.
CHECK(!HaveWidth || LeftJustified);
- Res += appendString(&Buffer, BufferEnd, LeftJustified ? -Width : Width,
- Precision, va_arg(Args, char *));
+ appendString(LeftJustified ? -Width : Width, Precision, va_arg(Args, char *));
break;
}
case 'c': {
RAW_CHECK_MSG(!HaveFlags, PrintfFormatsHelp);
- Res +=
- appendChar(&Buffer, BufferEnd, static_cast<char>(va_arg(Args, int)));
+ String.push_back(static_cast<char>(va_arg(Args, int)));
break;
}
// In Scudo, `s64`/`u64` are supposed to use `lld` and `llu` respectively.
@@ -207,19 +199,17 @@ static int formatString(char *Buffer, uptr BufferLength, const char *Format,
if (*Cur == 'd') {
DVal = va_arg(Args, s64);
- Res +=
- appendSignedDecimal(&Buffer, BufferEnd, DVal, Width, PadWithZero);
+ appendSignedDecimal(DVal, Width, PadWithZero);
} else {
UVal = va_arg(Args, u64);
- Res += appendUnsigned(&Buffer, BufferEnd, UVal, 10, Width, PadWithZero,
- false);
+ appendUnsigned(UVal, 10, Width, PadWithZero, false);
}
break;
}
case '%': {
RAW_CHECK_MSG(!HaveFlags, PrintfFormatsHelp);
- Res += appendChar(&Buffer, BufferEnd, '%');
+ String.push_back('%');
break;
}
default: {
@@ -227,35 +217,13 @@ static int formatString(char *Buffer, uptr BufferLength, const char *Format,
}
}
}
- RAW_CHECK(Buffer <= BufferEnd);
- appendChar(&Buffer, BufferEnd + 1, '\0');
- return Res;
-}
-
-int formatString(char *Buffer, uptr BufferLength, const char *Format, ...) {
- va_list Args;
- va_start(Args, Format);
- int Res = formatString(Buffer, BufferLength, Format, Args);
- va_end(Args);
- return Res;
-}
-
-void ScopedString::vappend(const char *Format, va_list Args) {
- va_list ArgsCopy;
- va_copy(ArgsCopy, Args);
- // formatString doesn't currently support a null buffer or zero buffer length,
- // so in order to get the resulting formatted string length, we use a one-char
- // buffer.
- char C[1];
- const uptr AdditionalLength =
- static_cast<uptr>(formatString(C, sizeof(C), Format, Args)) + 1;
- const uptr Length = length();
- String.resize(Length + AdditionalLength);
- const uptr FormattedLength = static_cast<uptr>(formatString(
- String.data() + Length, String.size() - Length, Format, ArgsCopy));
- RAW_CHECK(data()[length()] == '\0');
- RAW_CHECK(FormattedLength + 1 == AdditionalLength);
- va_end(ArgsCopy);
+ String.push_back('\0');
+ if (String.back() != '\0') {
+ // String truncated, make sure the string is terminated properly.
+ // This can happen if there is no more memory when trying to resize
+ // the string.
+ String.back() = '\0';
+ }
}
void ScopedString::append(const char *Format, ...) {
diff --git a/compiler-rt/lib/scudo/standalone/string_utils.h b/compiler-rt/lib/scudo/standalone/string_utils.h
index a4cab5268ede4a..d5405bb8d16ca4 100644
--- a/compiler-rt/lib/scudo/standalone/string_utils.h
+++ b/compiler-rt/lib/scudo/standalone/string_utils.h
@@ -25,17 +25,22 @@ class ScopedString {
String.clear();
String.push_back('\0');
}
- void vappend(const char *Format, va_list Args);
+ void vappend(const char *Format, va_list &Args);
void append(const char *Format, ...) FORMAT(2, 3);
void output() const { outputRaw(String.data()); }
void reserve(size_t Size) { String.reserve(Size + 1); }
+ uptr capacity() { return String.capacity() - 1; }
private:
+ void appendNumber(u64 AbsoluteValue, u8 Base, u8 MinNumberLength, bool PadWithZero, bool Negative, bool Upper);
+ void appendUnsigned(u64 Num, u8 Base, u8 MinNumberLength, bool PadWithZero, bool Upper);
+ void appendSignedDecimal(s64 Num, u8 MinNumberLength, bool PadWithZero);
+ void appendString(int Width, int MaxChars, const char *S);
+ void appendPointer(u64 ptr_value);
+
Vector<char> String;
};
-int formatString(char *Buffer, uptr BufferLength, const char *Format, ...)
- FORMAT(3, 4);
void Printf(const char *Format, ...) FORMAT(1, 2);
} // namespace scudo
diff --git a/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp b/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
index 7a69ffd9762c52..7cc107f7b17e8e 100644
--- a/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
@@ -66,6 +66,10 @@ TEST(ScudoStringsTest, Precision) {
Str.append("%-6s", "12345");
EXPECT_EQ(Str.length(), strlen(Str.data()));
EXPECT_STREQ("12345 ", Str.data());
+ Str.clear();
+ Str.append("%-8s", "12345");
+ EXPECT_EQ(Str.length(), strlen(Str.data()));
+ EXPECT_STREQ("12345 ", Str.data());
}
static void fillString(scudo::ScopedString &Str, scudo::uptr Size) {
@@ -123,3 +127,31 @@ TEST(ScudoStringsTest, Padding) {
testAgainstLibc<int>("%03d - %03d", 12, 1234);
testAgainstLibc<int>("%03d - %03d", -12, -1234);
}
+
+#if defined(__linux__)
+#include <sys/resource.h>
+
+TEST(ScudoStringsTest, CapacityIncreaseFails) {
+ scudo::ScopedString Str;
+
+ rlimit Limit = {};
+ EXPECT_EQ(0, getrlimit(RLIMIT_AS, &Limit));
+ rlimit EmptyLimit = {.rlim_max=Limit.rlim_max};
+ EXPECT_EQ(0, setrlimit(RLIMIT_AS, &EmptyLimit));
+
+ // Test requires that the default length is at least 6 characters.
+ scudo::uptr MaxSize = Str.capacity();
+ EXPECT_LE(6, MaxSize);
+
+ for (size_t i = 0; i < MaxSize - 5; i++) {
+ Str.append("B");
+ }
+
+ // Attempt to append past the end of the current capacity.
+ Str.append("%d", 12345678);
+ EXPECT_EQ(MaxSize, Str.capacity());
+ EXPECT_STREQ("B12345", &Str.data()[MaxSize - 6]);
+
+ EXPECT_EQ(0, setrlimit(RLIMIT_AS, &Limit));
+}
+#endif
diff --git a/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp b/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
index dc23c2a3471310..e16bb8452f7e46 100644
--- a/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
@@ -41,3 +41,38 @@ TEST(ScudoVectorTest, ResizeReduction) {
V.resize(1);
EXPECT_EQ(V.size(), 1U);
}
+
+#if defined(__linux__)
+
+#include <sys/resource.h>
+
+// Verify that if the reallocate fails, nothing new is added.
+TEST(ScudoVectorTest, ReallocateFails) {
+ scudo::Vector<char> V;
+ scudo::uptr capacity = V.capacity();
+
+ // Get the current address space size.
+ rlimit Limit = {};
+ EXPECT_EQ(0, getrlimit(RLIMIT_AS, &Limit));
+
+ rlimit EmptyLimit = { .rlim_max = Limit.rlim_max};
+ EXPECT_EQ(0, setrlimit(RLIMIT_AS, &EmptyLimit));
+
+ V.resize(capacity);
+ // Set the last element so we can check it later.
+ V.back() = '\0';
+
+ // The reallocate should fail, so the capacity should not change.
+ V.reserve(capacity + 1000);
+ EXPECT_EQ(capacity, V.capacity());
+
+ // Now try to do a push back and verify that the size does not change.
+ scudo::uptr Size = V.size();
+ V.push_back('2');
+ EXPECT_EQ(Size, V.size());
+ // Verify that the last element in the vector did not change.
+ EXPECT_EQ('\0', V.back());
+
+ EXPECT_EQ(0, setrlimit(RLIMIT_AS, &Limit));
+}
+#endif
diff --git a/compiler-rt/lib/scudo/standalone/vector.h b/compiler-rt/lib/scudo/standalone/vector.h
index c0f1ba0eddfa91..255c7647139a3c 100644
--- a/compiler-rt/lib/scudo/standalone/vector.h
+++ b/compiler-rt/lib/scudo/standalone/vector.h
@@ -35,7 +35,10 @@ template <typename T> class VectorNoCtor {
DCHECK_LE(Size, capacity());
if (Size == capacity()) {
const uptr NewCapacity = roundUpPowerOfTwo(Size + 1);
- reallocate(NewCapacity);
+ if (!reallocate(NewCapacity)) {
+ // Silently discard the data, otherwise the whole system will crash.
+ return;
+ }
}
memcpy(&Data[Size++], &Element, sizeof(T));
}
@@ -51,14 +54,17 @@ template <typename T> class VectorNoCtor {
const T *data() const { return Data; }
T *data() { return Data; }
constexpr uptr capacity() const { return CapacityBytes / sizeof(T); }
- void reserve(uptr NewSize) {
+ bool reserve(uptr NewSize) {
// Never downsize internal buffer.
if (NewSize > capacity())
- reallocate(NewSize);
+ return reallocate(NewSize);
+ return true;
}
void resize(uptr NewSize) {
if (NewSize > Size) {
- reserve(NewSize);
+ if (!reserve(NewSize)) {
+ return;
+ }
memset(&Data[Size], 0, sizeof(T) * (NewSize - Size));
}
Size = NewSize;
@@ -86,13 +92,15 @@ template <typename T> class VectorNoCtor {
}
private:
- void reallocate(uptr NewCapacity) {
+ bool reallocate(uptr NewCapacity) {
DCHECK_GT(NewCapacity, 0);
DCHECK_LE(Size, NewCapacity);
MemMapT NewExternalBuffer;
NewCapacity = roundUp(NewCapacity * sizeof(T), getPageSizeCached());
- NewExternalBuffer.map(/*Addr=*/0U, NewCapacity, "scudo:vector");
+ if (!NewExternalBuffer.map(/*Addr=*/0U, NewCapacity, "scudo:vector", MAP_ALLOWNOMEM)) {
+ return false;
+ }
T *NewExternalData = reinterpret_cast<T *>(NewExternalBuffer.getBase());
memcpy(NewExternalData, Data, Size * sizeof(T));
@@ -101,6 +109,7 @@ template <typename T> class VectorNoCtor {
Data = NewExternalData;
CapacityBytes = NewCapacity;
ExternalBuffer = NewExternalBuffer;
+ return true;
}
T *Data = nullptr;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
Comment on lines
+38
to
+40
| if (!reallocate(NewCapacity)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can we have another function like try_push_back to support this behavior and only silence the on that path? And/Or we can specialize the Vector to use try_push_back so that other OOM usage on other Vector instantiations can still be detected.
There was a problem hiding this comment.
Just came up with another idea, we can have something like StaticVector which uses a local array and never grows the buffer. So that in an OOM situation, we don't even bother allocating a single page for error message (given the error messages are short in general)
There was a problem hiding this comment.
There aren't any other uses of Vector, which is why I left it like this. I do agree that having a fixed string is a good idea. Would you mind if I did that as a follow-up rather than change this one? I think that will be a slightly bigger change since I need to tear up the ScopedString a bit to do it right.
There was a problem hiding this comment.
Sure, I'm fine with a follow-up of doing that. It does better to separate the further change.
ChiaHungDuan
approved these changes
Mar 26, 2024
|
@cferris1000 This is causing a -Wmissing-designated-field-initializers (treated as error) on: https://lab.llvm.org/buildbot/#/builders/57/builds/33825 |
RKSimon
reviewed
Mar 27, 2024
|
|
||
| rlimit Limit = {}; | ||
| EXPECT_EQ(0, getrlimit(RLIMIT_AS, &Limit)); | ||
| rlimit EmptyLimit = {.rlim_max = Limit.rlim_max}; |
There was a problem hiding this comment.
@cferris1000 I'm assuming we need something like this (not near a linux box atm to test it)
rlimit EmptyLimit = {.rlim_cur = 0, .rlim_max = Limit.rlim_max};
There was a problem hiding this comment.
Applying this change locally:
diff --git a/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp b/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
index e068c48fc97c..d31ca26d53a1 100644
--- a/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
@@ -136,7 +136,7 @@ TEST(ScudoStringsTest, CapacityIncreaseFails) {
rlimit Limit = {};
EXPECT_EQ(0, getrlimit(RLIMIT_AS, &Limit));
- rlimit EmptyLimit = {.rlim_max = Limit.rlim_max};
+ rlimit EmptyLimit = {.rlim_cur = 0, .rlim_max = Limit.rlim_max};
EXPECT_EQ(0, setrlimit(RLIMIT_AS, &EmptyLimit));
// Test requires that the default length is at least 6 characters.
diff --git a/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp b/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
index b7678678d8a2..add62c5a42a3 100644
--- a/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
@@ -55,7 +55,7 @@ TEST(ScudoVectorTest, ReallocateFails) {
rlimit Limit = {};
EXPECT_EQ(0, getrlimit(RLIMIT_AS, &Limit));
- rlimit EmptyLimit = {.rlim_max = Limit.rlim_max};
+ rlimit EmptyLimit = {.rlim_cur = 0, .rlim_max = Limit.rlim_max};
EXPECT_EQ(0, setrlimit(RLIMIT_AS, &EmptyLimit));
V.resize(capacity);Appears to resolve the issue.
There was a problem hiding this comment.
I committed 685d785 to resolve this.
There was a problem hiding this comment.
Thanks for fixing this.
|
This bot is still broken https://lab.llvm.org/buildbot/#/builders/169/builds/29947 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Do not abort if a vector cannot increase its own capacity. In that case, push_back calls silently fail.
Modify the ScopedString implementation so that it no longer requires two passes to do the format. Move the helper functions to be private member functions so that they can use push_back directly. This allows the capacity to be increased under the hood and/or silently discards data if the capacity is exceeded and cannot be increased.
Add new tests for the Vector and ScopedString for capacity increase failures.
Doing this so that if a map call fails, and we are attempting to write an error string, we can still get some of the message dumped. This also avoids crashing in Scudo code, and makes the caller handle any failures.