Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
The macros defined in sanitizer/asan_interface.h and in our header are the same. For consistency I just re-defined them. When the header is available, since it looks like the macros have been there for a long time, we can just skip redefining it and use them directly. I'll add a comment. On Tue, Jan 10, 2023 at 9:09 AM Ioi Lam wrote: > On Mon, 9 Jan 2023 15:48:48 GMT, Justin King wrote: > > >> I like this, but would compilers not complain about unused statements? > > > > I don't believe so, at least not based on current options. Other > projects use similar tricks, like ABSL_DCHECK from Abseil which short > circuits expressions when in non-debug builds causing the code to be > unreachable and stripped. If it does, we can always just remove the if > statement part and have it evaluate anyway. Most compilers should realize > that the statements are unused and should be pruned. > > The macros `ASAN_POISON_MEMORY_REGION` and `ASAN_UNPOISON_MEMORY_REGION` > are already defined in [sanitizer/asan_interface.h]( > https://opensource.apple.com/source/clang/clang-700.1.81/src/projects/compiler-rt/include/sanitizer/asan_interface.h.auto.html), > but we redefine them here. > > Since ASan is going to be an obscured feature for many HotSpot developers, > I don't think we should make it more obscured. > > I think it's better to declare our own macros that call into the existing > macros. Maybe something like > `_ASAN_POISON_MEMORY_REGION`. The comments should be improved to say why > we do this (to prevent people that don't use ASan from breaking ASan). > > - > > PR: https://git.openjdk.org/jdk/pull/11702 > -- [image: Google Logo] Justin King Software Engineer jck...@google.com smime.p7s Description: S/MIME Cryptographic Signature
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Tue, 10 Jan 2023 17:05:48 GMT, Ioi Lam wrote: >> I don't believe so, at least not based on current options. Other projects >> use similar tricks, like ABSL_DCHECK from Abseil which short circuits >> expressions when in non-debug builds causing the code to be unreachable and >> stripped. If it does, we can always just remove the if statement part and >> have it evaluate anyway. Most compilers should realize that the statements >> are unused and should be pruned. > > The macros `ASAN_POISON_MEMORY_REGION` and `ASAN_UNPOISON_MEMORY_REGION` are > already defined in > [sanitizer/asan_interface.h](https://opensource.apple.com/source/clang/clang-700.1.81/src/projects/compiler-rt/include/sanitizer/asan_interface.h.auto.html), > but we redefine them here. > > Since ASan is going to be an obscured feature for many HotSpot developers, I > don't think we should make it more obscured. > > I think it's better to declare our own macros that call into the existing > macros. Maybe something like > `_ASAN_POISON_MEMORY_REGION`. The comments should be improved to say why we > do this (to prevent people that don't use ASan from breaking ASan). I changed it to just use the macros directly when they are available instead of redefining it. When ASan is not present, both macros are equivalent to that provided by the header when ASan is not available. So it is not obscured. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Mon, 9 Jan 2023 15:48:48 GMT, Justin King wrote: >> I like this, but would compilers not complain about unused statements? > > I don't believe so, at least not based on current options. Other projects use > similar tricks, like ABSL_DCHECK from Abseil which short circuits expressions > when in non-debug builds causing the code to be unreachable and stripped. If > it does, we can always just remove the if statement part and have it evaluate > anyway. Most compilers should realize that the statements are unused and > should be pruned. The macros `ASAN_POISON_MEMORY_REGION` and `ASAN_UNPOISON_MEMORY_REGION` are already defined in [sanitizer/asan_interface.h](https://opensource.apple.com/source/clang/clang-700.1.81/src/projects/compiler-rt/include/sanitizer/asan_interface.h.auto.html), but we redefine them here. Since ASan is going to be an obscured feature for many HotSpot developers, I don't think we should make it more obscured. I think it's better to declare our own macros that call into the existing macros. Maybe something like `_ASAN_POISON_MEMORY_REGION`. The comments should be improved to say why we do this (to prevent people that don't use ASan from breaking ASan). - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Tue, 10 Jan 2023 06:57:01 GMT, David Holmes wrote: >> This doesn't look "too terrible", but I can't comment on the actual >> poisoning strategies. >> >> Cheers. > >> Forcing 2 reviewers to ensure @dholmes-ora can chime in before moving >> forward. > > Well I won't be able to Review as not familiar enough with the code, so > you'll need a second reviewer anyway. I don't hate this to the point of > outright rejecting it but I do have general concerns about whether we should > be directly supporting such tools in our codebase, and if we should whether > these are the right tools. So I've asked other hotspot folk to chime in. > > > Forcing 2 reviewers to ensure @dholmes-ora can chime in before moving > > > forward. > > > > > > Well I won't be able to Review as not familiar enough with the code, so > > you'll need a second reviewer anyway. I don't hate this to the point of > > outright rejecting it but I do have general concerns about whether we > > should be directly supporting such tools in our codebase, and if we should > > whether these are the right tools. So I've asked other hotspot folk to > > chime in. > > I dislike this too. I wondered whether we could hide it behind an "interface > for asan-like tools", where we have a `os::poison_memory(range)` and > `os::unpoison_memory(range)` function. Those functions could in turn call > whatever tool is configured. At least then we don't pollute the code base > with tool specifics. > > Granted, it sounds a bit fig leafy as long as there is only Asan. But if we > wanted, we could implement a primitive poisoner tool in hotspot by > mprotecting if the range spans whole pages. ASan is really one of a kind in this respect. I am not aware of any other tools like it, due to it requiring compiler support. GCC, Clang, and now [MSVC](https://learn.microsoft.com/en-us/cpp/sanitizers/asan-building?view=msvc-170) all support ASan. Given its wide adoption and touching all major platforms, it's a bit of a de facto standard at this point. It's also noted on MSVC [docs](https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=msvc-170) that additional sanitizers may be supported in the future as well. Also since we are using macros anyway (i.e. `ASAN_POISON_MEMORY_REGION`), it would be fairly easy to swap out the implementation to be something else if wanted such as the suggested primitive poisoner. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Tue, 10 Jan 2023 06:57:01 GMT, David Holmes wrote: > > Forcing 2 reviewers to ensure @dholmes-ora can chime in before moving > > forward. > > Well I won't be able to Review as not familiar enough with the code, so > you'll need a second reviewer anyway. I don't hate this to the point of > outright rejecting it but I do have general concerns about whether we should > be directly supporting such tools in our codebase, and if we should whether > these are the right tools. So I've asked other hotspot folk to chime in. I dislike this too. I wondered whether we could hide it behind an "interface for asan-like tools", where we have a `os::poison_memory(range)` and `os::unpoison_memory(range)` function. Those functions could in turn call whatever tool is configured. At least then we don't pollute the code base with tool specifics. Granted, it sounds a bit fig leafy as long as there is only Asan. But if we wanted, we could implement a primitive poisoner tool in hotspot by mprotecting if the range spans whole pages. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Wed, 4 Jan 2023 06:22:11 GMT, David Holmes wrote: >> Justin King has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Exclude more zapping when ASan is in use >> >> Signed-off-by: Justin King > > This doesn't look "too terrible", but I can't comment on the actual poisoning > strategies. > > Cheers. > Forcing 2 reviewers to ensure @dholmes-ora can chime in before moving forward. Well I won't be able to Review as not familiar enough with the code, so you'll need a second reviewer anyway. I don't hate this to the point of outright rejecting it but I do have general concerns about whether we should be directly supporting such tools in our codebase, and if we should whether these are the right tools. So I've asked other hotspot folk to chime in. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Wed, 4 Jan 2023 06:22:11 GMT, David Holmes wrote: >> Justin King has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Exclude more zapping when ASan is in use >> >> Signed-off-by: Justin King > > This doesn't look "too terrible", but I can't comment on the actual poisoning > strategies. > > Cheers. Forcing 2 reviewers to ensure @dholmes-ora can chime in before moving forward. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Mon, 9 Jan 2023 08:05:47 GMT, Thomas Stuefe wrote: >> Ah I see. > > I like this, but would compilers not complain about unused statements? I don't believe so, at least not based on current options. Other projects use similar tricks, like ABSL_DCHECK from Abseil which short circuits expressions when in non-debug builds causing the code to be unreachable and stripped. If it does, we can always just remove the if statement part and have it evaluate anyway. Most compilers should realize that the statements are unused and should be pruned. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Thu, 5 Jan 2023 02:53:54 GMT, David Holmes wrote: >> This helps with maintenance. All builds will still compile the `addr` and >> `size` statements, ensuring they are valid, but will strip them when ASan is >> not enabled. In the event that refactoring occurs and one of the statements >> becomes invalid due to a method renaming for example, but the refactorer >> misses it, the build will still fail. >> >> Mostly to help avoid requiring people to build with ASan to ensure it still >> works. > > Ah I see. I like this, but would compilers not complain about unused statements? - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Wed, 4 Jan 2023 13:55:33 GMT, Justin King wrote: >> src/hotspot/share/sanitizers/address.h line 56: >> >>> 54: #else >>> 55: // NOOP implementation which preserves the arguments, ensuring they >>> still compile, but ensures they >>> 56: // are stripped due to being unreachable. >> >> Why is this necessary? > > This helps with maintenance. All builds will still compile the `addr` and > `size` statements, ensuring they are valid, but will strip them when ASan is > not enabled. In the event that refactoring occurs and one of the statements > becomes invalid due to a method renaming for example, but the refactorer > misses it, the build will still fail. > > Mostly to help avoid requiring people to build with ASan to ensure it still > works. Ah I see. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Wed, 4 Jan 2023 06:21:26 GMT, David Holmes wrote: >> Justin King has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Exclude more zapping when ASan is in use >> >> Signed-off-by: Justin King > > src/hotspot/share/sanitizers/address.h line 56: > >> 54: #else >> 55: // NOOP implementation which preserves the arguments, ensuring they >> still compile, but ensures they >> 56: // are stripped due to being unreachable. > > Why is this necessary? This helps with maintenance. All builds will still compile the `addr` and `size` statements, ensuring they are valid, but will strip them when ASan is not enabled. In the event that refactoring occurs and one of the statements becomes invalid due to a method renaming for example, but the refactorer misses it, the build will still fail. Mostly to help avoid requiring people to build with ASan to ensure it still works. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Wed, 4 Jan 2023 06:00:36 GMT, David Holmes wrote: >> Justin King has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Exclude more zapping when ASan is in use >> >> Signed-off-by: Justin King > > src/hotspot/share/runtime/os.cpp line 949: > >> 947: // parent stack frames, read outside of initialized memory, and etc. So >> we tell ASan to not >> 948: // instrument this function. >> 949: NO_SANITIE_ADDRESS void os::print_hex_dump(outputStream* st, address >> start, address end, > > Typo: NO_SANITIE_ADDRESS ->NO_SANITIZE_ADDRESS ? Yep, fixed typo. > src/hotspot/share/sanitizers/address.h line 2: > >> 1: /* >> 2: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. > > Oracle didn't write this, it should have your/your-company's copyright. Ah, okay. Updated. > src/hotspot/share/sanitizers/address.h line 44: > >> 42: #endif >> 43: #else >> 44: #define NO_SANITIE_ADDRESS > > Typos Whoops, thanks. - PR: https://git.openjdk.org/jdk/pull/11702
Re: RFR: JDK-8298908: Instrument Metaspace for ASan [v2]
On Sat, 17 Dec 2022 06:48:13 GMT, Justin King wrote: >> This change instruments Metaspace for ASan. Metaspace allocates memory using >> `mmap`/`munmap` which ASan is not aware of. Fortunately ASan supports >> applications [manually poisoning/unpoisoning >> memory](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning). >> ASan is able to detect poisoned memory, similar to `use-after-free`, and >> will raise an error similarly called `use-after-poison`. This provides and >> extra layer of defense and confidence. >> >> The header `sanitizers/address.h` defines macros for poisoning/unpoisoning >> memory regions. These macros can be used regardless of build mode. When ASan >> is not available, they are implemented using a NOOP approach which still >> compiles the arguments but does so such that they will be stripped out by >> the compiler due to being unreachable. This helps with maintenance. >> >> This also has the added benefit of making >> [LSan](https://bugs.openjdk.org/browse/JDK-8298445) more accurate and >> deterministic, as LSan will not look for pointers to malloc memory in >> poisoned memory regions. >> >> IMO the benefit of doing this greatly outweighs the cost. > > Justin King has updated the pull request incrementally with one additional > commit since the last revision: > > Exclude more zapping when ASan is in use > > Signed-off-by: Justin King This doesn't look "too terrible", but I can't comment on the actual poisoning strategies. Cheers. src/hotspot/share/runtime/os.cpp line 949: > 947: // parent stack frames, read outside of initialized memory, and etc. So > we tell ASan to not > 948: // instrument this function. > 949: NO_SANITIE_ADDRESS void os::print_hex_dump(outputStream* st, address > start, address end, Typo: NO_SANITIE_ADDRESS ->NO_SANITIZE_ADDRESS ? src/hotspot/share/sanitizers/address.h line 2: > 1: /* > 2: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. Oracle didn't write this, it should have your/your-company's copyright. src/hotspot/share/sanitizers/address.h line 44: > 42: #endif > 43: #else > 44: #define NO_SANITIE_ADDRESS Typos src/hotspot/share/sanitizers/address.h line 56: > 54: #else > 55: // NOOP implementation which preserves the arguments, ensuring they still > compile, but ensures they > 56: // are stripped due to being unreachable. Why is this necessary? - PR: https://git.openjdk.org/jdk/pull/11702
