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

[David Holmes]

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

Sorry I think this is well-intentioned but unnecessary in nearly all cases. If you pass a char there is no potential problem. Only passing an actual int could be a problem.

src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 664:

> 662:     char after_key = line[key_len];
> 663:     if (strncmp(line, key, key_len) == 0
> 664:           && os::is_space(after_key) != 0

I think this is excessive. The value is a char so cannot be a problem. The only calls to isspace that need checking are those that actually pass an int, and even then there could be adequate safeguards in place.

src/hotspot/os/linux/os_linux.cpp line 1356:

> 1354:       if (s) {
> 1355:         // Skip blank chars
> 1356:         do { s++; } while (s && os::is_space(*s));

Again not needed.

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

PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2103234054
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630265925
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630266490