On Wed, 22 Nov 2023 16:24:24 GMT, suchismith1993 <d...@openjdk.org> wrote:
>> J2SE agent does not start and throws error when it tries to find the shared
>> library ibm_16_am.
>> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load
>> fails.It fails to identify the shared library ibm_16_am.a shared archive
>> file on AIX.
>> Hence we are providing a function which will additionally search for .a file
>> on AIX ,when the search for .so file fails.
>
> suchismith1993 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change macro position
I'm not a big fan of this approach. We accumulate more and more "#ifdef AIX" in
shared code because of many recent AIX additions. No other platform has such a
large ifdef footprint in shared code.
I argue that all of this should be handled inside os_aix.cpp and not leak out
into the external space:
If .a is a valid shared object format on AIX, this should be handled in
`os::dll_load()`, and be done for all shared objects. If not, why do we try to
load a static archive via dlload in this case but not in other cases?
*If* this is needed in shared code, the string replacement function should be a
generic utility function for all platforms, and it should be tested with a
small gtest. A gtest would have likely uncovered the buffer overflow too.
src/hotspot/os/aix/os_aix.cpp line 3065:
> 3063: //Replaces provided path with alternate path for the given file,if it
> doesnt exist.
> 3064: //For AIX,this replaces .so with .a.
> 3065: void os::Aix::mapAlternateName(char* buffer, const char *extension) {
The documentation is wrong:
// Replaces the specified path with an alternative path for the given file if
the original path doesn't exist
It does no such thing, it replaces the extension unconditionally. The comment
sounds like it does a file system check. That does not happen here.
The whole function is not well named - "map alternate name" does not really
tell me anything, I need to look at the implementation and the caller to
understand what it is doing. There is no mapping here, this is just a string
utility function.
The function should not modify the original buffer but instead assemble a copy.
That is the conventional way to do these things. You can work with immutable
strings as input, e.g. literals, and don't risk buffer overflows.
All of this should be handled inside os_aix.cpp; see my other comment. This
should not live in the external os::aix interface, since it has nothing to do
with AIX. *If* this is needed in generic code, which I don't think, then this
should be made generic utility API, available on all platforms, and with a
small gtest.
But I think all of this should be confined to os_aix.cpp.
Proposal for a clearer name, comment, and pseudocode
// Given a filename with an extension, return a new string containing the
filename with the new extension.
// New string is allocated in resource area.
static char* replace_extension_in_filename(const char* filename, const char*
new_extension) {
- allocate buffer in RA
- assemble new path by contacting old path - old extension + new extension
- return new path
}
src/hotspot/os/aix/os_aix.cpp line 3069:
> 3067: while (end > 0 && buffer[end] != '.') {
> 3068: end = end - 1;
> 3069: }
Use strrchr.
src/hotspot/os/aix/os_aix.cpp line 3072:
> 3070: buffer[end] = '\0';
> 3071: strcat(buffer, extension);
> 3072: }
This is a buffer overrun waiting to happen if replacement is larger than
original extension.
-------------
Changes requested by stuefe (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1745743102
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1402944936
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1402941240
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1402941730
