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

[David Holmes]

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 was about to revert this and just add a comment, but something was nagging at me and I realized that the original Windows version actually contains a benign bug. Where we have:

 if (len <= (prefix_len + suffix_len)) {

`len` is the original length of  `lib_name` before we stripped off the path components so we are checking the wrong value. This means that potentially `name_len` could be negative when we do:

name_len = strlen(lib_name) - suffix_len

with a worst-case value of -4. But this is benign because when we recalculate `len` we add back `strlen(sym_name)` which is larger than 4 and so we just lookup a bogus symbol which naturally can't be found.

Even though benign I feel the need to fix this. :)  So I'm going to bite the bullet and fix all of this, incorporating Aleksey's suggestions, and creating a single shared copy of the code.

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

PR Comment: https://git.openjdk.org/jdk/pull/22404#issuecomment-2505271077