On Thu, 14 May 2026 18:06:11 GMT, Weijun Wang <[email protected]> wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> comments
>
> src/java.base/share/classes/java/security/PEM.java line 167:
>
>> 165: this.type = type;
>> 166: final var c = content;
>> 167: CleanerFactory.cleaner().register(this, this::clear);
>
> Not a `Cleaner` expert, but I heard `this` cannot be referenced in the
> cleaner action.
Hmm.. I hadn't heard that, but I could see the point. Easy enough to just be
save and call KeyUtil.clear()
> src/java.base/share/classes/java/security/PEM.java line 202:
>
>> 200: * {@link Base64#getMimeDecoder()}.
>> 201: *
>> 202: * @return the Base64-decoded content
>
> Suggestion: return a copy of...
I had someone else recommend removing "copy of", since this is the decoded
content from getMimeDecoder().
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 379:
>
>> 377: Objects.requireNonNull(password, "a password must be
>> specified");
>> 378: Objects.requireNonNull(algorithm, "an algorithm must be
>> specified");
>> 379: char[] passwd = password.clone();
>
> Why clone here?
In the past some reviewers want clones in case the user-passed array changes
before it's used soon in the method. If you think this caution is not needed
that's fine with me.
> src/java.base/share/classes/sun/security/util/Pem.java line 320:
>
>> 318: } while (hyphen < 5);
>> 319:
>> 320: while ((c = is.read()) != eol && c != -1 && c != '\s' && c
>> != '\t') {
>
> This is different from reading the header. Space and tab are not skipped,
> instead, they end the loop. This means a block ending with `-----END
> SOMETHING------ xyz` is treated as valid.
The way I read RFC 7468,
```
textualmsg = preeb *WSP eol
*eolWSP
base64text
posteb *WSP [eol]
`\s` , `\t` and maybe `[eol]` are part of the footer otherwise they would not
be listed above. I also interpreted, but wasn't certain, that `[eol]` was
optional as it maybe the EOF. But if it was not EOF, then there would be an
`eol`. If I cannot assume the last line is `eol` or EOF, then I will read one
byte past the pem, in your example `x` and cannot push the char back into the
inputstream.
If I interpret the way you see it, `readPEM()` stops after the last dash, and
the next inputstream read may read the remainder of that line, which would be
awkward for `leadingData` or an app that reads inputstream directly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3244592797
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3244602143
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3244814074
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3245685321
