RFR: 8371762: Incorrect use of checked_cast in Arguments::process_settings_file [v5]

[Kim Barrett]

On Mon, 12 Jan 2026 13:28:32 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> [JDK-8313882](https://bugs.openjdk.org/browse/JDK-8313882) introduced a `checked_cast` when to silence the Wconversion warnings when we store an `int` in a `char`. The problem is that the assumption that the value in the `int` we get from `getc` is compatible with `char` is incorrect.
>> 
>> The specification for `getc` is that it returns either `EOF` or the next read `unsigned char` converted to an `int`. This means we have the possible values of `{EOF, 0-255}`, we always check for `EOF` first, so we end up only having `{0-255}` as the possible values. (While a char has the possible values of `{-128-127}`.)
>> 
>> So first the problem with `checked_cast` (which ensures that the type roundtrip is lossless) will end up converting any value in `{128-255}` to `{-128, -1}` which when converted back to an `int` is `{-128, -1}` and not `{128-255}`.
>> 
>> The thing is that this is not a problem except for the assert as we do not care if the conversion roundtrip is lossless. We only want to reinterpret the value as a `char`, and never try to regain the unsigned representation converted to an `int` that `getc` uses.
>> 
>> Another issue with keeping the value inside an `int` which could lead to bugs in the future is that when we use it in equality comparison agains a `char` we will encounter an integer promotion of the `char` which will lead to surprising incorrect results if the character is non-ascii.
>> 
>> Currently we have no such comparisons of this `int` vs a non-ascii character. So the only issue is the assert inside `checked_cast` which will trigger if the settings file contains a non-ascii character.
>> 
>> I suggest we convert the return value from `getc` to a `char` after we have check for `EOF` and before we use it as a character.
>> 
>> It is worth noting that the `(unsigned char)` casts for the calls into `isspace` (introduced by [JDK-8332400](https://bugs.openjdk.org/browse/JDK-8332400)) is very important to get the correct value for its `int` input parameter. (So we get the correct non-sign extended cast `char -> unsigned char -> int`.) That issue mentions the possibility of introducing our own `isspace` (suggestion was `os::isspace`) which we could overload the different types and do the correct thing with bound checks. This might be something we want to revisit.
>> 
>> _`isspace` specification:_
>>>The behavior is undefined if the value of ch is not representable as unsigned char and is not equal to [EOF](https://en.cppreference.com/w/c/io.html).
>> 
>> * Testin...
>
> Axel Boldt-Christmas 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 eight additional commits since the last revision:
> 
>  - Change type of pos to size_t
>  - Use static_cast
>  - Update copyright year
>  - Merge remote-tracking branch 'upstream_jdk/master' into JDK-8371762
>  - Merge tag 'jdk-27+3' into JDK-8371762
>    
>    Added tag jdk-27+3 for changeset 4f283f18
>  - Merge tag 'jdk-26+26' into JDK-8371762
>    
>    Added tag jdk-26+26 for changeset 847fbab7
>  - Update src/hotspot/share/runtime/arguments.cpp
>    
>    Co-authored-by: David Holmes <62092539+dholmes-ora at users.noreply.github.com>
>  - 8371762: Incorrect use of checked_cast in Arguments::process_settings_file

Never mind - the half-formed idea I had didn't pan out.  Approved.

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

Marked as reviewed by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/28283#pullrequestreview-3657108055