[ASan][ADT] Don't scribble with ASan #79066

AdvenamTacet · 2024-01-22T22:18:32Z

AddressSanitizer (ASAN) disables scribbling to prevent overwriting poisoned objects.
Needed by #79049

AddressSanitizer (ASAN) disables scribbling to prevent overwriting poisoned objects. Needed by llvm#79049

llvmbot · 2024-01-22T22:19:05Z

@llvm/pr-subscribers-llvm-adt

Author: Tacet (AdvenamTacet)

Changes

AddressSanitizer (ASAN) disables scribbling to prevent overwriting poisoned objects.
Needed by #79049

Full diff: https://github.com/llvm/llvm-project/pull/79066.diff

1 Files Affected:

(modified) llvm/include/llvm/ADT/FunctionExtras.h (+4-2)

diff --git a/llvm/include/llvm/ADT/FunctionExtras.h b/llvm/include/llvm/ADT/FunctionExtras.h
index 4cf1de488c7bde2..c58d0263b2efa07 100644
--- a/llvm/include/llvm/ADT/FunctionExtras.h
+++ b/llvm/include/llvm/ADT/FunctionExtras.h
@@ -317,8 +317,10 @@ template <typename ReturnT, typename... ParamTs> class UniqueFunctionBase {
     // Clear the old callback and inline flag to get back to as-if-null.
     RHS.CallbackAndInlineFlag = {};
 
-#ifndef NDEBUG
-    // In debug builds, we also scribble across the rest of the storage.
+#if !defined(NDEBUG) && !(defined(ADDRESS_SANITIZER) || defined(__SANITIZE_ADDRESS__))
+    // In debug builds without ASan, we also scribble across the rest of the storage.
+    // AddressSanitizer (ASAN) disables scribbling to prevent overwriting poisoned objects
+    // (e.g., annotated short strings).
     memset(RHS.getInlineStorage(), 0xAD, InlineStorageSize);
 #endif
   }

github-actions · 2024-01-22T22:21:02Z

✅ With the latest revision this PR passed the C/C++ code formatter.

aeubanks · 2024-01-22T22:30:02Z

use LLVM_ADDRESS_SANITIZER_BUILD instead from https://github.com/llvm/llvm-project/blob/fe0ec2c91cfbf2aad2a61e402f21b771db685b90/llvm/include/llvm/Support/Compiler.h#L442C10-L442C39

AdvenamTacet · 2024-01-22T22:49:49Z

@aeubanks thx! Just updated my PRs.

aeubanks · 2024-01-22T23:38:06Z

code lgtm with comment change

llvm/include/llvm/ADT/FunctionExtras.h

This commit: - adds #include "llvm/Support/Compiler.h" - fixes incorrect use of LLVM_ADDRESS_SANITIZER_BUILD

AdvenamTacet · 2024-01-23T05:37:26Z

code lgtm with comment change

I don't see any comment from you, should I change something more? Let me know and if not, please approve the review.

This commit fixes incorrect use of `LLVM_ADDRESS_SANITIZER_BUILD` macro. Details here: llvm#79066 (comment)

AdvenamTacet · 2024-01-23T15:57:20Z

I ran buildbot_bootstrap_asan locally with that change and all ASan tests passed.

llvm/include/llvm/ADT/FunctionExtras.h

aeubanks · 2024-01-22T23:37:35Z

llvm/include/llvm/ADT/FunctionExtras.h

-#ifndef NDEBUG
-    // In debug builds, we also scribble across the rest of the storage.
+#if !defined(NDEBUG) && !defined(LLVM_ADDRESS_SANITIZER_BUILD)
+    // In debug builds without ASan, we also scribble across the rest of the


nit, I'd reword the second sentence to say Disable scribbling under ASan to... since ASan isn't disabling it, we're disabling it under ASan

I fixed the text to be more precise. Thx!

aeubanks · 2024-01-23T17:57:32Z

ughhhh I thought I hit comment, so sorry

edit: oh I think the comment at the very bottom of "conversation" is different from a code review

Originally merged here: #75882 Reverted here: #78627 Reverted due to failing buildbots. The problem was not caused by the annotations code, but by code in the `UniqueFunctionBase` class and in the `JSON.h` file. That code caused the program to write to memory that was already being used by string objects, which resulted in an ASan error. Fixes are implemented in: - #79065 - #79066 Problematic code from `UniqueFunctionBase` for example: ```cpp #ifndef NDEBUG // In debug builds, we also scribble across the rest of the storage. memset(RHS.getInlineStorage(), 0xAD, InlineStorageSize); #endif ``` --- Original description: This commit turns on ASan annotations in `std::basic_string` for short stings (SSO case). Originally suggested here: https://reviews.llvm.org/D147680 String annotations added here: #72677 Requires to pass CI without fails: - #75845 - #75858 Annotating `std::basic_string` with default allocator is implemented in #72677 but annotations for short strings (SSO - Short String Optimization) are turned off there. This commit turns them on. This also removes `_LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED`, because we do not plan to support turning on and off short string annotations. Support in ASan API exists since dd1b7b7. You can turn off annotations for a specific allocator based on changes from 2fa1bec. This PR is a part of a series of patches extending AddressSanitizer C++ container overflow detection capabilities by adding annotations, similar to those existing in `std::vector` and `std::deque` collections. These enhancements empower ASan to effectively detect instances where the instrumented program attempts to access memory within a collection's internal allocation that remains unused. This includes cases where access occurs before or after the stored elements in `std::deque`, or between the `std::basic_string`'s size (including the null terminator) and capacity bounds. The introduction of these annotations was spurred by a real-world software bug discovered by Trail of Bits, involving an out-of-bounds memory access during the comparison of two strings using the `std::equals` function. This function was taking iterators (`iter1_begin`, `iter1_end`, `iter2_begin`) to perform the comparison, using a custom comparison function. When the `iter1` object exceeded the length of `iter2`, an out-of-bounds read could occur on the `iter2` object. Container sanitization, upon enabling these annotations, would effectively identify and flag this potential vulnerability. If you have any questions, please email: advenam.tacet@trailofbits.com disconnect3d@trailofbits.com

Originally merged here: #75882 Reverted here: #78627 Reverted due to failing buildbots. The problem was not caused by the annotations code, but by code in the `UniqueFunctionBase` class and in the `JSON.h` file. That code caused the program to write to memory that was already being used by string objects, which resulted in an ASan error. Fixes are implemented in: - #79065 - #79066 Problematic code from `UniqueFunctionBase` for example: ```cpp #ifndef NDEBUG // In debug builds, we also scribble across the rest of the storage. memset(RHS.getInlineStorage(), 0xAD, InlineStorageSize); #endif ```

This is 3rd attempt to upstream short string annotations, it's the same as the previous one, but other PRs fixed issues withing LLVM: - llvm#79489 - llvm#79522 Additionaly annotations were updated (but it shouldn't have any impact on anything): - llvm#79292 Now, as far as I know, all buildbots should work without problems. Both previous reverts were not related to issues with string annotations, but with issues in LLVM/clang. Read PRs above and below for details. --- Previous description: Originally merged here: llvm#75882 Reverted here: llvm#78627 Reverted due to failing buildbots. The problem was not caused by the annotations code, but by code in the `UniqueFunctionBase` class and in the `JSON.h` file. That code caused the program to write to memory that was already being used by string objects, which resulted in an ASan error. Fixes are implemented in: - llvm#79065 - llvm#79066 Problematic code from `UniqueFunctionBase` for example: ```cpp // In debug builds, we also scribble across the rest of the storage. memset(RHS.getInlineStorage(), 0xAD, InlineStorageSize); ``` --- Original description: This commit turns on ASan annotations in `std::basic_string` for short stings (SSO case). Originally suggested here: https://reviews.llvm.org/D147680 String annotations added here: llvm#72677 Requires to pass CI without fails: - llvm#75845 - llvm#75858 Annotating `std::basic_string` with default allocator is implemented in llvm#72677 but annotations for short strings (SSO - Short String Optimization) are turned off there. This commit turns them on. This also removes `_LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED`, because we do not plan to support turning on and off short string annotations. Support in ASan API exists since llvm@dd1b7b7. You can turn off annotations for a specific allocator based on changes from llvm@2fa1bec. This PR is a part of a series of patches extending AddressSanitizer C++ container overflow detection capabilities by adding annotations, similar to those existing in `std::vector` and `std::deque` collections. These enhancements empower ASan to effectively detect instances where the instrumented program attempts to access memory within a collection's internal allocation that remains unused. This includes cases where access occurs before or after the stored elements in `std::deque`, or between the `std::basic_string`'s size (including the null terminator) and capacity bounds. The introduction of these annotations was spurred by a real-world software bug discovered by Trail of Bits, involving an out-of-bounds memory access during the comparison of two strings using the `std::equals` function. This function was taking iterators (`iter1_begin`, `iter1_end`, `iter2_begin`) to perform the comparison, using a custom comparison function. When the `iter1` object exceeded the length of `iter2`, an out-of-bounds read could occur on the `iter2` object. Container sanitization, upon enabling these annotations, would effectively identify and flag this potential vulnerability. If you have any questions, please email: advenam.tacet@trailofbits.com disconnect3d@trailofbits.com

Originally merged here: #75882 Reverted here: #78627 Reverted due to failing buildbots. The problem was not caused by the annotations code, but by code in the `UniqueFunctionBase` class and in the `JSON.h` file. That code caused the program to write to memory that was already being used by string objects, which resulted in an ASan error. Fixes are implemented in: - #79065 - #79066 Problematic code from `UniqueFunctionBase` for example: ```cpp // In debug builds, we also scribble across the rest of the storage. memset(RHS.getInlineStorage(), 0xAD, InlineStorageSize); ```

Originally merged here: llvm/llvm-project#75882 Reverted here: llvm/llvm-project#78627 Reverted due to failing buildbots. The problem was not caused by the annotations code, but by code in the `UniqueFunctionBase` class and in the `JSON.h` file. That code caused the program to write to memory that was already being used by string objects, which resulted in an ASan error. Fixes are implemented in: - llvm/llvm-project#79065 - llvm/llvm-project#79066 Problematic code from `UniqueFunctionBase` for example: ```cpp #ifndef NDEBUG // In debug builds, we also scribble across the rest of the storage. memset(RHS.getInlineStorage(), 0xAD, InlineStorageSize); #endif ``` --- Original description: This commit turns on ASan annotations in `std::basic_string` for short stings (SSO case). Originally suggested here: https://reviews.llvm.org/D147680 String annotations added here: llvm/llvm-project#72677 Requires to pass CI without fails: - llvm/llvm-project#75845 - llvm/llvm-project#75858 Annotating `std::basic_string` with default allocator is implemented in llvm/llvm-project#72677 but annotations for short strings (SSO - Short String Optimization) are turned off there. This commit turns them on. This also removes `_LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED`, because we do not plan to support turning on and off short string annotations. Support in ASan API exists since llvm/llvm-project@dd1b7b7. You can turn off annotations for a specific allocator based on changes from llvm/llvm-project@2fa1bec. This PR is a part of a series of patches extending AddressSanitizer C++ container overflow detection capabilities by adding annotations, similar to those existing in `std::vector` and `std::deque` collections. These enhancements empower ASan to effectively detect instances where the instrumented program attempts to access memory within a collection's internal allocation that remains unused. This includes cases where access occurs before or after the stored elements in `std::deque`, or between the `std::basic_string`'s size (including the null terminator) and capacity bounds. The introduction of these annotations was spurred by a real-world software bug discovered by Trail of Bits, involving an out-of-bounds memory access during the comparison of two strings using the `std::equals` function. This function was taking iterators (`iter1_begin`, `iter1_end`, `iter2_begin`) to perform the comparison, using a custom comparison function. When the `iter1` object exceeded the length of `iter2`, an out-of-bounds read could occur on the `iter2` object. Container sanitization, upon enabling these annotations, would effectively identify and flag this potential vulnerability. If you have any questions, please email: advenam.tacet@trailofbits.com disconnect3d@trailofbits.com NOKEYCHECK=True GitOrigin-RevId: cb528ec5e6331ce207c7b835d7ab963bd5e13af7

This is 3rd attempt to upstream short string annotations, it's the same as the previous one, but other PRs fixed issues withing LLVM: - llvm#79489 - llvm#79522 Additionaly annotations were updated (but it shouldn't have any impact on anything): - llvm#79292 Now, as far as I know, all buildbots should work without problems. Both previous reverts were not related to issues with string annotations, but with issues in LLVM/clang. Read PRs above and below for details. --- Previous description: Originally merged here: llvm#75882 Reverted here: llvm#78627 Reverted due to failing buildbots. The problem was not caused by the annotations code, but by code in the `UniqueFunctionBase` class and in the `JSON.h` file. That code caused the program to write to memory that was already being used by string objects, which resulted in an ASan error. Fixes are implemented in: - llvm#79065 - llvm#79066 Problematic code from `UniqueFunctionBase` for example: ```cpp // In debug builds, we also scribble across the rest of the storage. memset(RHS.getInlineStorage(), 0xAD, InlineStorageSize); ``` --- Original description: This commit turns on ASan annotations in `std::basic_string` for short stings (SSO case). Originally suggested here: https://reviews.llvm.org/D147680 String annotations added here: llvm#72677 Requires to pass CI without fails: - llvm#75845 - llvm#75858 Annotating `std::basic_string` with default allocator is implemented in llvm#72677 but annotations for short strings (SSO - Short String Optimization) are turned off there. This commit turns them on. This also removes `_LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED`, because we do not plan to support turning on and off short string annotations. Support in ASan API exists since llvm@dd1b7b7. You can turn off annotations for a specific allocator based on changes from llvm@2fa1bec. This PR is a part of a series of patches extending AddressSanitizer C++ container overflow detection capabilities by adding annotations, similar to those existing in `std::vector` and `std::deque` collections. These enhancements empower ASan to effectively detect instances where the instrumented program attempts to access memory within a collection's internal allocation that remains unused. This includes cases where access occurs before or after the stored elements in `std::deque`, or between the `std::basic_string`'s size (including the null terminator) and capacity bounds. The introduction of these annotations was spurred by a real-world software bug discovered by Trail of Bits, involving an out-of-bounds memory access during the comparison of two strings using the `std::equals` function. This function was taking iterators (`iter1_begin`, `iter1_end`, `iter2_begin`) to perform the comparison, using a custom comparison function. When the `iter1` object exceeded the length of `iter2`, an out-of-bounds read could occur on the `iter2` object. Container sanitization, upon enabling these annotations, would effectively identify and flag this potential vulnerability. If you have any questions, please email: advenam.tacet@trailofbits.com disconnect3d@trailofbits.com

@vitalybuka

This pull request is the third iteration aiming to integrate short string annotations. This commit includes: - Enabling basic_string annotations for short strings. - Setting a value of `__trivially_relocatable` in `std::basic_string` to `false_type` when compiling with ASan (nothing changes when compiling without ASan). Short string annotations make `std::basic_string` to not be trivially relocatable, because memory has to be unpoisoned. - Adding a `_LIBCPP_STRING_INTERNAL_MEMORY_ACCESS` modifier to two functions. - Creating a macro `_LIBCPP_ASAN_VOLATILE_WRAPPER` to prevent problematic stack optimizations (the macro modifies code behavior only when compiling with ASan). Previously we had issues with compiler optimization, which we understand thanks to @vitalybuka. This commit also addresses smaller changes in short string, since previous upstream attempts. Problematic optimization was loading two values in code similar to: ``` __is_long() ? __get_long_size() : __get_short_size(); ``` We aim to resolve it with the volatile wrapper. This commit is built on top of two previous attempts which descriptions are below. Additionally, in the meantime, annotations were updated (but it shouldn't have any impact on anything): - #79292 --- Previous PR: #79049 Reverted: a16f81f Previous description: Originally merged here: #75882 Reverted here: #78627 Reverted due to failing buildbots. The problem was not caused by the annotations code, but by code in the `UniqueFunctionBase` class and in the `JSON.h` file. That code caused the program to write to memory that was already being used by string objects, which resulted in an ASan error. Fixes are implemented in: - #79065 - #79066 Problematic code from `UniqueFunctionBase` for example: ```cpp // In debug builds, we also scribble across the rest of the storage. memset(RHS.getInlineStorage(), 0xAD, InlineStorageSize); ``` --- Original description: This commit turns on ASan annotations in `std::basic_string` for short stings (SSO case). Originally suggested here: https://reviews.llvm.org/D147680 String annotations added here: #72677 Requires to pass CI without fails: - #75845 - #75858 Annotating `std::basic_string` with default allocator is implemented in #72677 but annotations for short strings (SSO - Short String Optimization) are turned off there. This commit turns them on. This also removes `_LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED`, because we do not plan to support turning on and off short string annotations. Support in ASan API exists since dd1b7b7. You can turn off annotations for a specific allocator based on changes from 2fa1bec. This PR is a part of a series of patches extending AddressSanitizer C++ container overflow detection capabilities by adding annotations, similar to those existing in `std::vector` and `std::deque` collections. These enhancements empower ASan to effectively detect instances where the instrumented program attempts to access memory within a collection's internal allocation that remains unused. This includes cases where access occurs before or after the stored elements in `std::deque`, or between the `std::basic_string`'s size (including the null terminator) and capacity bounds. The introduction of these annotations was spurred by a real-world software bug discovered by Trail of Bits, involving an out-of-bounds memory access during the comparison of two strings using the `std::equals` function. This function was taking iterators (`iter1_begin`, `iter1_end`, `iter2_begin`) to perform the comparison, using a custom comparison function. When the `iter1` object exceeded the length of `iter2`, an out-of-bounds read could occur on the `iter2` object. Container sanitization, upon enabling these annotations, would effectively identify and flag this potential vulnerability. If you have any questions, please email: - advenam.tacet@trailofbits.com - disconnect3d@trailofbits.com

[ASan][ADT] Don't scribble with ASan

21c85d9

AddressSanitizer (ASAN) disables scribbling to prevent overwriting poisoned objects. Needed by llvm#79049

AdvenamTacet added this to the LLVM 18.0.X Release milestone Jan 22, 2024

AdvenamTacet requested review from vitalybuka and aeubanks January 22, 2024 22:18

AdvenamTacet assigned vitalybuka Jan 22, 2024

llvmbot added the llvm:adt label Jan 22, 2024

AdvenamTacet mentioned this pull request Jan 22, 2024

[ASan][libc++] Turn on ASan annotations for short strings #79049

Merged

clang-format-fix

97c4c59

Use LLVM_ADDRESS_SANITIZER_BUILD

4d19c7e

vitalybuka reviewed Jan 23, 2024

View reviewed changes

llvm/include/llvm/ADT/FunctionExtras.h Outdated Show resolved Hide resolved

Fix based on code review

9072fba

This commit: - adds #include "llvm/Support/Compiler.h" - fixes incorrect use of LLVM_ADDRESS_SANITIZER_BUILD

AdvenamTacet requested a review from vitalybuka January 23, 2024 05:31

AdvenamTacet pushed a commit to trail-of-forks/llvm-project that referenced this pull request Jan 23, 2024

Fix use of LLVM_ADDRESS_SANITIZER_BUILD

442bc63

This commit fixes incorrect use of `LLVM_ADDRESS_SANITIZER_BUILD` macro. Details here: llvm#79066 (comment)

vitalybuka reviewed Jan 23, 2024

View reviewed changes

llvm/include/llvm/ADT/FunctionExtras.h Show resolved Hide resolved

vitalybuka approved these changes Jan 23, 2024

View reviewed changes

aeubanks approved these changes Jan 23, 2024

View reviewed changes

Update comment text.

9651e4b

AdvenamTacet force-pushed the adt-no-scribble branch from 55ec3e4 to 9651e4b Compare January 23, 2024 18:02

AdvenamTacet merged commit 51a11f1 into llvm:main Jan 23, 2024
3 of 4 checks passed

AdvenamTacet deleted the adt-no-scribble branch January 23, 2024 18:13

AdvenamTacet mentioned this pull request Jan 26, 2024

[ASan][libc++] Turn on ASan annotations for short strings #79536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASan][ADT] Don't scribble with ASan #79066

[ASan][ADT] Don't scribble with ASan #79066

AdvenamTacet commented Jan 22, 2024

llvmbot commented Jan 22, 2024

github-actions bot commented Jan 22, 2024 •

edited

Loading

aeubanks commented Jan 22, 2024

AdvenamTacet commented Jan 22, 2024

aeubanks commented Jan 22, 2024

AdvenamTacet commented Jan 23, 2024

AdvenamTacet commented Jan 23, 2024

aeubanks Jan 22, 2024

AdvenamTacet Jan 23, 2024

aeubanks commented Jan 23, 2024 •

edited

Loading

[ASan][ADT] Don't scribble with ASan #79066

[ASan][ADT] Don't scribble with ASan #79066

Conversation

AdvenamTacet commented Jan 22, 2024

llvmbot commented Jan 22, 2024

github-actions bot commented Jan 22, 2024 • edited Loading

aeubanks commented Jan 22, 2024

AdvenamTacet commented Jan 22, 2024

aeubanks commented Jan 22, 2024

AdvenamTacet commented Jan 23, 2024

AdvenamTacet commented Jan 23, 2024

aeubanks Jan 22, 2024

Choose a reason for hiding this comment

AdvenamTacet Jan 23, 2024

Choose a reason for hiding this comment

aeubanks commented Jan 23, 2024 • edited Loading

github-actions bot commented Jan 22, 2024 •

edited

Loading

aeubanks commented Jan 23, 2024 •

edited

Loading