On Thu, 24 Apr 2025 20:04:19 GMT, Mark Reinhold <m...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - javadoc updates
>> - code review comments
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 78:
>
>> 76: *
>> 77: * <p> {@code String} values returned by this class use character set
>> 78: * {@link java.nio.charset.StandardCharsets#ISO_8859_1 ISO-8859-1}
>
> `String` values in Java are always encoded in UTF-16. Did you mean to write
> something like, "Byte streams consumed by methods in this class are assumed
> to represent characters encoded in the ISO-8859-1 charset" ?
Actually this comment isn't necessary as this method doesn't return Strings.
> src/java.base/share/classes/java/security/PEMDecoder.java line 84:
>
>> 82: * <pre>
>> 83: * PEMDecoder pd = PEMDecoder.of();
>> 84: * PrivateKey priKey = pd.decode(PriKeyPEM);
>
> s/PriKeyPEM/priKeyPEM/
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 213:
>
>> 211: Objects.requireNonNull(str);
>> 212: try {
>> 213: return decode(new ByteArrayInputStream(str.getBytes()));
>
> `str.getBytes()` will return a byte array encoded in the default charset,
> which these days is likely to be UTF-8, but might be something completely
> bizarre. You probably want `str.getBytes(StandardCharsets.ISO_8859_1)`.
>
> It could be worth running your unit tests in several different locales in
> order to catch similar issues.
What about the other decode methods that read from InputStream? I thought it
would be inconsistent to read `decode(String)` as a ISO 8859-1, but
`decode(InputStream)` as a different charset.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062695888
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062696020
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062701688
