RFR: 8345012: os::build_agent_function_name potentially wastes a byte when allocating the buffer [v3]

[Thomas Stuefe]

On Thu, 28 Nov 2024 05:01:22 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This is a minor issue noticed in the review of JEP 479. The calculations of the buffer length always do `+2` to allow for an optional underscore plus the terminator. A solution was suggested in that review (which I initially used) but I've instead opted for a solution where `name_len` always reflects the actual length of the lib name - otherwise we need to subtract the 1 again when doing the `strncat`. I also made the need for the underscore explicit. 
>> 
>> As the same code is used in os_posix.cpp both versions were made consistent. The only difference between them is the Windows version has to check for a drive specifier. I toyed with combining them into a shared version with a Windows-specific chunk, but opted for the simpler change. I can revisit that if people have strong opinions.
>> 
>> Testing
>> - tiers 1-4
>> 
>> Thanks
>
> David Holmes has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Merge branch 'master' into 8345012-windows
>  - Fix bug re-checking minimum length
>  - Rework things so that `name_len` is always actually the length of the lib name, and make using
>    an underscore explicit
>  - Fix up strncat length
>  - Same changes for os_posix and align code between the versions
>  - 8345012: os::build_agent_function_name potentially wastes a byte when allocating the buffer

I think there is a preexisting error in that `name_len`is fed as `size` parameter to `strncat`. Since the buffer is already partly filled at that point, we cannot use the full buffer size but should use the size of the remaining space in the buffer.

I feel all that headache could be easily avoided by using snprintf, or better stringStream. E.g.


stringStream ss(agent_entry_name, len);
ss.print_raw(sym_name);
if (libname != nullptr) {
  ss.put('_');
  ss.print_raw(libname);
}

src/hotspot/os/posix/os_posix.cpp line 976:

> 974:    // Total buffer length to allocate - includes null terminator.
> 975:   len = strlen(sym_name) + (need_underscore ? 1 : 0) + name_len + 1;
> 976:   agent_entry_name = NEW_C_HEAP_ARRAY_RETURN_NULL(char, len, mtThread);

Preexisting: If we fail to allocate memory, we return NULL, which results in various places in confusing "cannot find agent function" messages. Maybe better would be a clear native oom exit here?

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

PR Review: https://git.openjdk.org/jdk/pull/22404#pullrequestreview-2466826555
PR Review Comment: https://git.openjdk.org/jdk/pull/22404#discussion_r1861617715