On Wed, 5 Jun 2024 20:08:10 GMT, Robert Toyonaga <d...@openjdk.org> wrote:
> ### Summary > This change ensures we don't get undefined behavior when > calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html). > `isspace` accepts an `int` argument that "the application shall ensure is a > character representable as an unsigned char or equal to the value of the > macro EOF.". > > Previously, there was no checking of the values passed to `isspace`. I've > replaced direct calls with a new wrapper `os::is_space` that performs a range > check and prevents the possibility of undefined behavior from happening. For > instances outside of Hotspot, I've added casts to `unsigned char`. > > **Testing** > - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check > `os::is_space` is working correctly. > - tier1 Hi Robert, Here's a third opinion: The wrapper is fine, but it should mimic the name of the function it wraps exactly: `os::isspace`. It's also good that it does the range check as regular code as opposed to an assert, as this function is used to parse external input. It's fine to be excessive when parsing strings as those parts of the code are not really performance critical. All the best, Johan src/hotspot/share/runtime/os.cpp line 96: > 94: DEBUG_ONLY(bool os::_mutex_init_done = false;) > 95: > 96: int os::is_space(int c) { `os::isspace` test/hotspot/gtest/runtime/test_os.cpp line 43: > 41: > 42: # include <stdio.h> > 43: Not necessary. ------------- PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2107152152 PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1632873691 PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1632873309
