RFR: 8313396: Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION [v4]
Martin Doerr
mdoerr at openjdk.org
Tue Jan 7 15:21:39 UTC 2025
On Tue, 7 Jan 2025 09:09:16 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>> Please review this change to how HotSpot prevents the use of certain C library
>> functions (e.g. poisons references to those functions), while permitting a
>> subset to be used in restricted circumstances. Reasons for poisoning a
>> function include it being considered obsolete, or a security concern, or there
>> is a HotSpot function (typically in the os:: namespace) providing similar
>> functionality that should be used instead.
>>
>> The old mechanism, based on -Wattribute-warning and the associated attribute,
>> only worked for gcc. (Clang's implementation differs in an important way from
>> gcc, which is the subject of a clang bug that has been open for years. MSVC
>> doesn't provide a similar mechanism.) It also had problems with LTO, due to a
>> gcc bug.
>>
>> The new mechanism is based on deprecation warnings, using [[deprecated]]
>> attributes. We redeclare or forward declare the functions we want to prevent
>> use of as being deprecated. This relies on deprecation warnings being
>> enabled, which they already are in our build configuration. All of our
>> supported compilers support the [[deprecated]] attribute.
>>
>> Another benefit of using deprecation warnings rather than warning attributes
>> is the time when the check is performed. Warning attributes are checked only
>> if the function is referenced after all optimizations have been performed.
>> Deprecation is checked during initial semantic analysis. That's better for
>> our purposes here. (This is also part of why gcc LTO has problems with the
>> old mechanism, but not the new.)
>>
>> Adding these redeclarations or forward declarations isn't as simple as
>> expected, due to differences between the various compilers. We hide the
>> differences behind a set of macros, FORBID_C_FUNCTION and related macros. See
>> the compiler-specific parts of those macros for details.
>>
>> In some situations we need to allow references to these poisoned functions.
>>
>> One common case is where our poisoning is visible to some 3rd party code we
>> don't want to modify. This is typically 3rd party headers included in HotSpot
>> code, such as from Google Test or the C++ Standard Library. For these the
>> BEGIN/END_ALLOW_FORBIDDEN_FUNCTIONS pair of macros are used demark the context
>> where such references are permitted.
>>
>> Some of the poisoned functions are needed to implement associated HotSpot os::
>> functions, or in other similarly restricted contexts. For these, a wrapper
>> function is provided that calls the poison...
>
> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
>
> more fixes for clang noreturn issues
Thanks! That solved the issue. Now, we need to adapt all usages in the low level AIX code and support `strdup`:
diff --git a/src/hotspot/os/aix/libodm_aix.cpp b/src/hotspot/os/aix/libodm_aix.cpp
index 9fe0fb7abd8..854fd5e2b79 100644
--- a/src/hotspot/os/aix/libodm_aix.cpp
+++ b/src/hotspot/os/aix/libodm_aix.cpp
@@ -30,6 +30,7 @@
#include <string.h>
#include "runtime/arguments.hpp"
#include "runtime/os.hpp"
+#include "utilities/permitForbiddenFunctions.hpp"
dynamicOdm::dynamicOdm() {
@@ -59,7 +60,7 @@ dynamicOdm::~dynamicOdm() {
}
-void odmWrapper::clean_data() { if (_data) { free(_data); _data = nullptr; } }
+void odmWrapper::clean_data() { if (_data) { permit_forbidden_function::free(_data); _data = nullptr; } }
int odmWrapper::class_offset(const char *field, bool is_aix_5)
diff --git a/src/hotspot/os/aix/loadlib_aix.cpp b/src/hotspot/os/aix/loadlib_aix.cpp
index bc21aef3836..88f660ad46f 100644
--- a/src/hotspot/os/aix/loadlib_aix.cpp
+++ b/src/hotspot/os/aix/loadlib_aix.cpp
@@ -38,6 +38,7 @@
#include "logging/log.hpp"
#include "utilities/debug.hpp"
#include "utilities/ostream.hpp"
+#include "utilities/permitForbiddenFunctions.hpp"
// For loadquery()
#include <sys/ldr.h>
@@ -55,7 +56,7 @@ class StringList {
// Enlarge list. If oom, leave old list intact and return false.
bool enlarge() {
int cap2 = _cap + 64;
- char** l2 = (char**) ::realloc(_list, sizeof(char*) * cap2);
+ char** l2 = (char**) permit_forbidden_function::realloc(_list, sizeof(char*) * cap2);
if (!l2) {
return false;
}
@@ -73,7 +74,7 @@ class StringList {
}
}
assert0(_cap > _num);
- char* s2 = ::strdup(s);
+ char* s2 = permit_forbidden_function::strdup(s);
if (!s2) {
return nullptr;
}
@@ -167,7 +168,7 @@ static void free_entry_list(loaded_module_t** start) {
loaded_module_t* lm = *start;
while (lm) {
loaded_module_t* const lm2 = lm->next;
- ::free(lm);
+ permit_forbidden_function::free(lm);
lm = lm2;
}
*start = nullptr;
@@ -190,7 +191,7 @@ static bool reload_table() {
uint8_t* buffer = nullptr;
size_t buflen = 1024;
for (;;) {
- buffer = (uint8_t*) ::realloc(buffer, buflen);
+ buffer = (uint8_t*) permit_forbidden_function::realloc(buffer, buflen);
if (loadquery(L_GETINFO, buffer, buflen) == -1) {
if (errno == ENOMEM) {
buflen *= 2;
@@ -210,7 +211,7 @@ static bool reload_table() {
for (;;) {
- loaded_module_t* lm = (loaded_module_t*) ::malloc(sizeof(loaded_module_t));
+ loaded_module_t* lm = (loaded_module_t*) permit_forbidden_function::malloc(sizeof(loaded_module_t));
if (!lm) {
log_warning(os)("OOM.");
goto cleanup;
@@ -226,7 +227,7 @@ static bool reload_table() {
lm->path = g_stringlist.add(ldi->ldinfo_filename);
if (!lm->path) {
log_warning(os)("OOM.");
- free(lm);
+ permit_forbidden_function::free(lm);
goto cleanup;
}
@@ -248,7 +249,7 @@ static bool reload_table() {
lm->member = g_stringlist.add(p_mbr_name);
if (!lm->member) {
log_warning(os)("OOM.");
- free(lm);
+ permit_forbidden_function::free(lm);
goto cleanup;
}
} else {
@@ -296,7 +297,7 @@ static bool reload_table() {
free_entry_list(&new_list);
}
- ::free(buffer);
+ permit_forbidden_function::free(buffer);
return rc;
diff --git a/src/hotspot/os/aix/os_aix.cpp b/src/hotspot/os/aix/os_aix.cpp
index 26627c2f8fb..2d0859a4d5e 100644
--- a/src/hotspot/os/aix/os_aix.cpp
+++ b/src/hotspot/os/aix/os_aix.cpp
@@ -74,6 +74,7 @@
#include "utilities/defaultStream.hpp"
#include "utilities/events.hpp"
#include "utilities/growableArray.hpp"
+#include "utilities/permitForbiddenFunctions.hpp"
#include "utilities/vmError.hpp"
#if INCLUDE_JFR
#include "jfr/support/jfrNativeLibraryLoadEvent.hpp"
@@ -364,9 +365,9 @@ static void query_multipage_support() {
// or by environment variable LDR_CNTRL (suboption DATAPSIZE). If none is given,
// default should be 4K.
{
- void* p = ::malloc(16*M);
+ void* p = permit_forbidden_function::malloc(16*M);
g_multipage_support.datapsize = os::Aix::query_pagesize(p);
- ::free(p);
+ permit_forbidden_function::free(p);
}
// Query default shm page size (LDR_CNTRL SHMPSIZE).
@@ -1406,7 +1407,7 @@ static struct {
} vmem;
static void vmembk_add(char* addr, size_t size, size_t pagesize, int type) {
- vmembk_t* p = (vmembk_t*) ::malloc(sizeof(vmembk_t));
+ vmembk_t* p = (vmembk_t*) permit_forbidden_function::malloc(sizeof(vmembk_t));
assert0(p);
if (p) {
MiscUtils::AutoCritSect lck(&vmem.cs);
@@ -1435,7 +1436,7 @@ static void vmembk_remove(vmembk_t* p0) {
for (vmembk_t** pp = &(vmem.first); *pp; pp = &((*pp)->next)) {
if (*pp == p0) {
*pp = p0->next;
- ::free(p0);
+ permit_forbidden_function::free(p0);
return;
}
}
diff --git a/src/hotspot/os/aix/porting_aix.cpp b/src/hotspot/os/aix/porting_aix.cpp
index 9d91c91bf8a..2235d3da686 100644
--- a/src/hotspot/os/aix/porting_aix.cpp
+++ b/src/hotspot/os/aix/porting_aix.cpp
@@ -1082,7 +1082,7 @@ void* Aix_dlopen(const char* filename, int Flags, int *eno, const char** error_r
if (g_handletable_used == max_handletable) {
// No place in array anymore; increase array.
unsigned new_max = MAX2(max_handletable * 2, init_num_handles);
- struct handletableentry* new_tab = (struct handletableentry*)::realloc(p_handletable, new_max * sizeof(struct handletableentry));
+ struct handletableentry* new_tab = (struct handletableentry*) permit_forbidden_function::realloc(p_handletable, new_max * sizeof(struct handletableentry));
assert(new_tab != nullptr, "no more memory for handletable");
if (new_tab == nullptr) {
*error_report = "dlopen: no more memory for handletable";
diff --git a/src/hotspot/share/utilities/permitForbiddenFunctions.hpp b/src/hotspot/share/utilities/permitForbiddenFunctions.hpp
index 0611d7e1996..a751158832c 100644
--- a/src/hotspot/share/utilities/permitForbiddenFunctions.hpp
+++ b/src/hotspot/share/utilities/permitForbiddenFunctions.hpp
@@ -60,6 +60,7 @@ inline void* malloc(size_t size) { return ::malloc(size); }
inline void free(void* ptr) { return ::free(ptr); }
inline void* calloc(size_t nmemb, size_t size) { return ::calloc(nmemb, size); }
inline void* realloc(void* ptr, size_t size) { return ::realloc(ptr, size); }
+inline char* strdup(const char* str) { return ::strdup(str); }
END_ALLOW_FORBIDDEN_FUNCTIONS
} // namespace permit_forbidden_function
It may be possible to use some `os::` versions, but that would need a careful review. I think it's safer to keep it this way for now. @JoKern65: You may want to take a look, too.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/22890#issuecomment-2575556410
More information about the graal-dev
mailing list