RFR: 8332400: isspace argument should be a valid unsigned char

[Johan Sjölen]

On Wed, 5 Jun 2024 20:08:10 GMT, Robert Toyonaga <duke at 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