From 90e8f09bfb2c26e0706286b157acd0d37519d795 Mon Sep 17 00:00:00 2001 From: David Justo Date: Fri, 24 Jan 2025 13:01:38 -0800 Subject: [PATCH 1/9] manually modify ASan annotations on `reserve` and add unit test. Missing - make unit test XFAIL --- stl/inc/xstring | 8 ++++++++ .../std/tests/GH_002030_asan_annotate_string/test.cpp | 11 +++++++++++ 2 files changed, 19 insertions(+) diff --git a/stl/inc/xstring b/stl/inc/xstring index ea4393dae3d..a71ccf4088f 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2418,6 +2418,10 @@ public: [](_Elem* const _New_ptr, const _Elem* const _Old_ptr, const size_type _Old_size) _STATIC_CALL_OPERATOR { _Traits::copy(_New_ptr, _Old_ptr, _Old_size + 1); }); + // `_Reallocate_grow_by`calls `_ASAN_STRING_CREATE` assuming that the string + // has size (initialized memory) equal to it's new capacity (allocated memory). + // This is not true in `reserve`, so we modify the ASan annotation. + _ASAN_STRING_MODIFY(*this, _Mypair._Myval2._Mysize, _Old_size); _Mypair._Myval2._Mysize = _Old_size; } @@ -2442,6 +2446,10 @@ public: [](_Elem* const _New_ptr, const _Elem* const _Old_ptr, const size_type _Old_size) _STATIC_CALL_OPERATOR { _Traits::copy(_New_ptr, _Old_ptr, _Old_size + 1); }); + // `_Reallocate_grow_by`calls `_ASAN_STRING_CREATE` assuming that the string + // has size (initialized memory) equal to it's new capacity (allocated memory). + // This is not true in `reserve`, so we modify the ASan annotation. + _ASAN_STRING_MODIFY(*this, _Mypair._Myval2._Mysize, _Old_size); _Mypair._Myval2._Mysize = _Old_size; return; } diff --git a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp index 5d869884177..30eedafabf2 100644 --- a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp +++ b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp @@ -1917,6 +1917,16 @@ void test_gh_3955() { assert(s == t); } +void test_gh_5251() { + // GH-5251 : ASan annotations do not prevent writing to allocated + // but uninitialized basic_string memory + std::basic_string myString; + myString.reserve(100); + char* data = &myString[0]; + data[50] = 'A'; + // FIXME: How do I declare this as an expected failure? +} + int main() { run_allocator_matrix(); #ifdef __cpp_char8_t @@ -1930,4 +1940,5 @@ int main() { test_DevCom_10109507(); test_gh_3883(); test_gh_3955(); + test_gh_5251(); } From 7538633b4c6867c6efd484bc81a94ffc261b31ab Mon Sep 17 00:00:00 2001 From: David Justo Date: Fri, 24 Jan 2025 15:25:58 -0800 Subject: [PATCH 2/9] add death test --- .../GH_002030_asan_annotate_string/test.cpp | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp index 30eedafabf2..fcf4b871f01 100644 --- a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp +++ b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -1924,21 +1925,25 @@ void test_gh_5251() { myString.reserve(100); char* data = &myString[0]; data[50] = 'A'; - // FIXME: How do I declare this as an expected failure? } -int main() { - run_allocator_matrix(); -#ifdef __cpp_char8_t - run_allocator_matrix(); -#endif // __cpp_char8_t - run_allocator_matrix(); - run_allocator_matrix(); - run_allocator_matrix(); - - test_DevCom_10116361(); - test_DevCom_10109507(); - test_gh_3883(); - test_gh_3955(); - test_gh_5251(); +int main(int argc, char* argv[]) { + std_testing::death_test_executive exec([] { + run_allocator_matrix(); + #ifdef __cpp_char8_t + run_allocator_matrix(); + #endif // __cpp_char8_t + run_allocator_matrix(); + run_allocator_matrix(); + run_allocator_matrix(); + + test_DevCom_10116361(); + test_DevCom_10109507(); + test_gh_3883(); + test_gh_3955(); + }); +#ifdef __SANITIZE_ADDRESS__ + exec.add_death_tests({test_gh_5251}); +#endif // ASan instrumentation enabled + return exec.run(argc, argv); } From 281910eb62527b0498ced129edaabce48c2e9a5d Mon Sep 17 00:00:00 2001 From: David Justo Date: Fri, 24 Jan 2025 15:29:54 -0800 Subject: [PATCH 3/9] fix spacing in comment --- stl/inc/xstring | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index a71ccf4088f..258e9b796e7 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2418,9 +2418,9 @@ public: [](_Elem* const _New_ptr, const _Elem* const _Old_ptr, const size_type _Old_size) _STATIC_CALL_OPERATOR { _Traits::copy(_New_ptr, _Old_ptr, _Old_size + 1); }); - // `_Reallocate_grow_by`calls `_ASAN_STRING_CREATE` assuming that the string + // `_Reallocate_grow_by` calls `_ASAN_STRING_CREATE` assuming that the string // has size (initialized memory) equal to it's new capacity (allocated memory). - // This is not true in `reserve`, so we modify the ASan annotation. + // This is not true for the `reserve` method, so we modify the ASan annotation. _ASAN_STRING_MODIFY(*this, _Mypair._Myval2._Mysize, _Old_size); _Mypair._Myval2._Mysize = _Old_size; } @@ -2446,9 +2446,9 @@ public: [](_Elem* const _New_ptr, const _Elem* const _Old_ptr, const size_type _Old_size) _STATIC_CALL_OPERATOR { _Traits::copy(_New_ptr, _Old_ptr, _Old_size + 1); }); - // `_Reallocate_grow_by`calls `_ASAN_STRING_CREATE` assuming that the string + // `_Reallocate_grow_by` calls `_ASAN_STRING_CREATE` assuming that the string // has size (initialized memory) equal to it's new capacity (allocated memory). - // This is not true in `reserve`, so we modify the ASan annotation. + // This is not true for the `reserve` method, so we modify the ASan annotation. _ASAN_STRING_MODIFY(*this, _Mypair._Myval2._Mysize, _Old_size); _Mypair._Myval2._Mysize = _Old_size; return; From 756e2f49e1a9ed82dbf76b35b175148dd5e56446 Mon Sep 17 00:00:00 2001 From: David Justo Date: Fri, 24 Jan 2025 15:30:54 -0800 Subject: [PATCH 4/9] add comment to test --- tests/std/tests/GH_002030_asan_annotate_string/test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp index fcf4b871f01..3e61bb6fc0d 100644 --- a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp +++ b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp @@ -1924,7 +1924,7 @@ void test_gh_5251() { std::basic_string myString; myString.reserve(100); char* data = &myString[0]; - data[50] = 'A'; + data[50] = 'A'; // ASan should fire! } int main(int argc, char* argv[]) { From 6457226afa397f5948fd1de683e604001f34f0b2 Mon Sep 17 00:00:00 2001 From: David Justo Date: Fri, 24 Jan 2025 16:16:20 -0800 Subject: [PATCH 5/9] run: clang-format --- tests/std/tests/GH_002030_asan_annotate_string/test.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp index 3e61bb6fc0d..71593d79549 100644 --- a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp +++ b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp @@ -15,12 +15,13 @@ #include #include #include -#include #include #include #include #include #include + +#include #if _HAS_CXX17 #include #endif // _HAS_CXX17 @@ -1924,15 +1925,15 @@ void test_gh_5251() { std::basic_string myString; myString.reserve(100); char* data = &myString[0]; - data[50] = 'A'; // ASan should fire! + data[50] = 'A'; // ASan should fire! } int main(int argc, char* argv[]) { std_testing::death_test_executive exec([] { run_allocator_matrix(); - #ifdef __cpp_char8_t +#ifdef __cpp_char8_t run_allocator_matrix(); - #endif // __cpp_char8_t +#endif // __cpp_char8_t run_allocator_matrix(); run_allocator_matrix(); run_allocator_matrix(); From 3b8cdbdc2d5002918f07f03faf7d8660c36d0897 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 28 Jan 2025 13:52:32 -0800 Subject: [PATCH 6/9] Fix comment typos. --- stl/inc/xstring | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index 258e9b796e7..6d5954ea93f 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2419,7 +2419,7 @@ public: _STATIC_CALL_OPERATOR { _Traits::copy(_New_ptr, _Old_ptr, _Old_size + 1); }); // `_Reallocate_grow_by` calls `_ASAN_STRING_CREATE` assuming that the string - // has size (initialized memory) equal to it's new capacity (allocated memory). + // has size (initialized memory) equal to its new capacity (allocated memory). // This is not true for the `reserve` method, so we modify the ASan annotation. _ASAN_STRING_MODIFY(*this, _Mypair._Myval2._Mysize, _Old_size); _Mypair._Myval2._Mysize = _Old_size; @@ -2447,7 +2447,7 @@ public: _STATIC_CALL_OPERATOR { _Traits::copy(_New_ptr, _Old_ptr, _Old_size + 1); }); // `_Reallocate_grow_by` calls `_ASAN_STRING_CREATE` assuming that the string - // has size (initialized memory) equal to it's new capacity (allocated memory). + // has size (initialized memory) equal to its new capacity (allocated memory). // This is not true for the `reserve` method, so we modify the ASan annotation. _ASAN_STRING_MODIFY(*this, _Mypair._Myval2._Mysize, _Old_size); _Mypair._Myval2._Mysize = _Old_size; From 2c07dffc4ad4bfb969b94eab71dffc5177c2f015 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 28 Jan 2025 14:04:57 -0800 Subject: [PATCH 7/9] Drop redundant `std::` qualification. --- tests/std/tests/GH_002030_asan_annotate_string/test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp index 71593d79549..b1af1c32e56 100644 --- a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp +++ b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp @@ -1922,7 +1922,7 @@ void test_gh_3955() { void test_gh_5251() { // GH-5251 : ASan annotations do not prevent writing to allocated // but uninitialized basic_string memory - std::basic_string myString; + basic_string myString; myString.reserve(100); char* data = &myString[0]; data[50] = 'A'; // ASan should fire! From 5195babd7a340189f2b19f76817888320fd9039c Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 28 Jan 2025 14:05:46 -0800 Subject: [PATCH 8/9] Avoid shadowing: `data` => `myData` --- tests/std/tests/GH_002030_asan_annotate_string/test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp index b1af1c32e56..d8b9a0388cf 100644 --- a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp +++ b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp @@ -1924,8 +1924,8 @@ void test_gh_5251() { // but uninitialized basic_string memory basic_string myString; myString.reserve(100); - char* data = &myString[0]; - data[50] = 'A'; // ASan should fire! + char* myData = &myString[0]; + myData[50] = 'A'; // ASan should fire! } int main(int argc, char* argv[]) { From d0a9a4bd0f70e2b51e62c97732cc944faf3f60f7 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 28 Jan 2025 14:07:43 -0800 Subject: [PATCH 9/9] Simplify: `basic_string` => `string` --- tests/std/tests/GH_002030_asan_annotate_string/test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp index d8b9a0388cf..e8bf4d30c3e 100644 --- a/tests/std/tests/GH_002030_asan_annotate_string/test.cpp +++ b/tests/std/tests/GH_002030_asan_annotate_string/test.cpp @@ -1922,7 +1922,7 @@ void test_gh_3955() { void test_gh_5251() { // GH-5251 : ASan annotations do not prevent writing to allocated // but uninitialized basic_string memory - basic_string myString; + string myString; myString.reserve(100); char* myData = &myString[0]; myData[50] = 'A'; // ASan should fire!