From fbc30e914542cb3a452b01a0714b4e2dff2561a7 Mon Sep 17 00:00:00 2001 From: Kyle Ambroff-Kao Date: Tue, 31 Dec 2019 02:07:54 -0800 Subject: [PATCH] support/String: Revert 6c67c7d63 to fix double-free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 6c67c7d63 was attempting to fix a leak caught by a static analysis tool, but it actually just introduced a double-free bug. Running `UnitTester BString` will result in a crash. The original code was correct because, in the event that realloc() fails in BString::_Resize(), the value of fPrivateData is still retained. It will be freed by the destructor of BString only if fPrivateData is not shared by another BString instance, since BStrings are copy-on-write. Note that while the change in 6c67c7d63 caused tests to fail, that doesn't mean those tests are ideal. They only trigger BString::_Resize() to fail because they depend on implementation details of hoard2 which limits allocations via malloc() to 1GB. Most malloc() implementations will allow allocations of arbitrary sizes using anonymous mappings (mmap on Linux, or create_area() in Haiku). This is a much bigger change, so for now I'm just adding some comments so that we can revisit these tests if we make a change to the allocator. Change-Id: I208c1c7a76b6b4409d237b911c62bb3198e49dab Reviewed-on: https://review.haiku-os.org/c/haiku/+/2060 Reviewed-by: Stephan Aßmus --- src/kits/support/String.cpp | 9 ++------- src/tests/kits/support/bstring/StringAppendTest.cpp | 9 ++++++++- src/tests/kits/support/bstring/StringAssignTest.cpp | 8 ++++++++ 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/kits/support/String.cpp b/src/kits/support/String.cpp index 2089d62960..9bdfcbb280 100644 --- a/src/kits/support/String.cpp +++ b/src/kits/support/String.cpp @@ -2360,14 +2360,9 @@ BString::_Resize(int32 length) if (length < 0) length = 0; - char* newData = (char*)realloc(data, length + kPrivateDataOffset + 1); - if (newData == NULL) { - free(data); - data = NULL; + data = (char*)realloc(data, length + kPrivateDataOffset + 1); + if (data == NULL) return NULL; - } else { - data = newData; - } data += kPrivateDataOffset; diff --git a/src/tests/kits/support/bstring/StringAppendTest.cpp b/src/tests/kits/support/bstring/StringAppendTest.cpp index 130137553b..0cb65e727f 100644 --- a/src/tests/kits/support/bstring/StringAppendTest.cpp +++ b/src/tests/kits/support/bstring/StringAppendTest.cpp @@ -116,7 +116,14 @@ StringAppendTest::PerformTest(void) CPPUNIT_ASSERT(strcmp(str1->String(), "BaseCCCCC") == 0); delete str1; - + // TODO: The following test cases only work with hoard2, which will not + // allow allocations via malloc() larger than the largest size-class + // (see threadHeap::malloc(size_t). Other malloc implementations like + // rpmalloc will allow arbitrarily large allocations via create_area(). + // + // This test should be made more robust by breaking the dependency on + // the allocator to simulate failures in another way. This may require + // a tricky build configuration to avoid breaking the ABI of BString. #ifndef TEST_R5 const int32 OUT_OF_MEM_VAL = 2 * 1000 * 1000 * 1000; // Append(char, int32) with excessive length: diff --git a/src/tests/kits/support/bstring/StringAssignTest.cpp b/src/tests/kits/support/bstring/StringAssignTest.cpp index edb26e2f98..414c76492c 100644 --- a/src/tests/kits/support/bstring/StringAssignTest.cpp +++ b/src/tests/kits/support/bstring/StringAssignTest.cpp @@ -99,6 +99,14 @@ StringAssignTest::PerformTest(void) delete str; #ifndef TEST_R5 + // TODO: The following test cases only work with hoard2, which will not + // allow allocations via malloc() larger than the largest size-class + // (see threadHeap::malloc(size_t). Other malloc implementations like + // rpmalloc will allow arbitrarily large allocations via create_area(). + // + // This test should be made more robust by breaking the dependency on + // the allocator to simulate failures in another way. This may require + // a tricky build configuration to avoid breaking the ABI of BString. const int32 OUT_OF_MEM_VAL = 2 * 1000 * 1000 * 1000; // SetTo(char, int32) with excessive length: NextSubTest();