RFR: JDK-8289633: Forbid raw C-heap allocation functions in hotspot and fix findings [v3]

[Thomas Stuefe]

On Mon, 4 Jul 2022 13:31:43 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> [JDK-8214976](https://bugs.openjdk.org/browse/JDK-8214976) introduced a way to forbid functions from being called outside of explicitly allowed contexts. Kim [1] proposed to use that functionality to forbid raw malloc and friends. That would have prevented [JDK-8289477](https://bugs.openjdk.org/browse/JDK-8289477), which sneaked in raw malloc and free via C-runtime defined macros.
>> 
>> We forbid now all functions that return C-heap, even if that is only optional, like with `realpath`. Note that there may be more functions, but these are all I know from the top of my head. We forbid them even if they are exotic to prevent devs from using them in the future and also from creep-in via system macros.
>> 
>> I found a number of places where raw allocation functions were used, mostly strdup. I either changes those places to use os::xxx where I was confident that works, or where I saw we really must use the raw functions I marked them with ALLOW_C_FUNCTION.
>> 
>> Places that allow raw C functions:
>> - decoder on Linux, since the C++ demangler returns raw C heap
>> - realpath, in conjunction with allowing real free for the returned buffer
>> - ZGC uses posix_memalign for a static global buffer that never is deleted. Keeping to use posix_memalign is probably ok, but we should add an os::posix_memalign at some point
>> - UL, LogTagSet, since UL may also be used for logging inside NMT and we don't want circularities
>> - obviously os::malloc and friends
>> - NMT pre-initialization code because circularities
>> - In gtest main function - I think gtest should work always, even if os::malloc is broken.
>> 
>> Places I fixed:
>> - ZGC, mountpoint string handling
>> - In CompilerEvent we hold a global lookup table with phase names. The names in there leak, but this table never gets cleared, so I think that's okay
>> - gcLogPrecious, string is fed to VMError::report_and_die, so it probably does not matter
>> - there were several places in JVMCI, one where we ::strdup a string which we give to a new code blob as blob name. These strings actually leak. I opened https://bugs.openjdk.org/browse/JDK-8289632 to track this
>> - A couple of places in gtests.
>> 
>> Note, wherever I introduced os::xxx and had to add os.hpp, I commented the include with "//malloc" to earmark those in case we ever want to move os::malloc and friends into its own header.
>> 
>> ----
>> 
>> Tests: I build and ran gtests manually on x64 fastdebug, release, arm fastdebug, aarch64 fastdebug, x86 fastdebug (all Linux). I also tested build on Alpine x64.
>> 
>> GHAs are in work.
>> 
>> 
>> [1] https://mail.openjdk.org/pipermail/hotspot-dev/2022-July/061602.html
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Use our realpath() wrapper in os_perf_linux.cpp

Hi David,

> Hi Thomas,
> 
> Generally this looks okay. Not sure whether this will impact any of the NMT tests as we will now be using NMT in new places.

NMT tests are fine. The additions are minor. We may now see more leaks, since two of the places I adapted did not free the memory; so if Oracle has some sort of NMT-based leak testing internally, that may now fail.

> 
> At least one place where it isn't obvious there is a matching os::free for os::strdup.
> 
> Thanks.

Thanks David!

-------------

PR: https://git.openjdk.org/jdk/pull/9356