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

[Kim Barrett]

On Sun, 3 Jul 2022 08:04:09 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

A few small nits, but generally good.

One stylistic question is the naming of functions in ALLOW_xxx macro uses.  Sometimes they are explicitly in the global namespace (`::foo`), sometimes not, and sometimes the macro's name argument and the use(s) in the permissive code are different.  At least the latter probably should be made consistent.  Regularizing others might be worthwhile, in which case I suggest explicit global namespace qualification.  Not sure how strongly I feel about that.

src/hotspot/cpu/ppc/macroAssembler_ppc_sha.cpp line 28:

> 26: #ifdef AIX
> 27: #include "runtime/os.hpp" // malloc
> 28: #endif

Is it actually important to make the inclusion conditional?  Also, the Style Guide says conditional includes go at the end.

src/hotspot/share/runtime/os.cpp line 739:

> 737:   void* const old_outer_ptr = MemTracker::record_free(memblock);
> 738: 
> 739:   ALLOW_C_FUNCTION(::realloc, ::free(old_outer_ptr);)

s/realloc/free/ - unfortunately, the existing macro implementations only use the name for "documentation" purposes.

src/hotspot/share/utilities/globalDefinitions.hpp line 178:

> 176: FORBID_C_FUNCTION(void free(void *ptr), "use os::free");
> 177: FORBID_C_FUNCTION(void* realloc(void *ptr, size_t size), "use os::realloc");
> 178: FORBID_C_FUNCTION(char* strdup(const char *s), "use os::realloc");

s/realloc/strdup/

src/hotspot/share/utilities/globalDefinitions.hpp line 179:

> 177: FORBID_C_FUNCTION(void* realloc(void *ptr, size_t size), "use os::realloc");
> 178: FORBID_C_FUNCTION(char* strdup(const char *s), "use os::realloc");
> 179: FORBID_C_FUNCTION(char* strndup(const char *s, size_t n), "use os::strdup");

I take it there are no calls to `::strndup`?  If there were, we should probably add `os::strndup`.

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

Changes requested by kbarrett (Reviewer).

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