On Tue, 15 Feb 2022 15:40:42 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> src/java.base/share/classes/sun/security/x509/OtherName.java line 93:
>>
>>> 91: oid = in.getOID();
>>> 92: DerValue derValue1 = in.getDerValue();
>>> 93: if (!derValue1.isContextSpecific((byte)0) ||
>>> !derValue1.isConstructed()) {
>>
>> It might be purely a matter of taste, but isn't `!(isCSTag0 && isConst)`
>> easier to read?
>
> I have difficulty describing `!(a && b)`. There is no parentheses in human
> language and `!` has higher order than `&&`.
>
> I thought about completely reverse the block but that means everything after
> the throw will be inside a block and I don't want to move so many lines.
My wording for the &&: If the tag is not a constructed and context-specific tag
number 0, then an exception is thrown. The parens denote that both conditions
need to apply:
!(isCSTag0 && isConst)
true, true = !(true && true) = !true = false
true, false = !(true && false) = !false = true
false, true = !(false && true) = !false = true
false, false = !(false && false) = !false = true
!isCSTag0 || !isConst
true, true = !true || !true = false || false = false
true, false = !true || !false = false || true = true
false, true = !false || !true = true || false = true
false, false = !false || !false = true || true = true
>> src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 1594:
>>
>>> 1592: String v = new
>>> DerValue(nameValue).getAsString();
>>> 1593: nameEntry.add(v == null ? nameValue : v);
>>> 1594: } catch (IOException ioe) {
>>
>> Attention, this catch block will hide invalid ASN.1 encoding of the other
>> name element from:
>> * sun.security.util.DerValue.init(boolean, InputStream, boolean)
>> * sun.security.util.DerValue.getIA5String()
>>
>> Other blocks throw:
>>
>> throw new CertificateException("Unable to parse DER value of SAN:otherName",
>> ioe);
>>
>>
>> Do you really intend to hide an encoding error int the cert from the user?
>
> Up to debate. Other blocks in `makeAltNames` throw `RuntimeException`.
Correct, but they don't swallow at least.
>> test/jdk/sun/security/x509/OtherName/Parse.java line 89:
>>
>>> 87: int found = 0;
>>> 88: for (var san : x.getSubjectAlternativeNames()) {
>>> 89: if (san.size() == 4) {
>>
>> Make it `>=` if this is going to change in the future..
>
> OK, but this is always implementation-specific. Precisely, it's a "may" so I
> should not check the count.
Right, let's leave as-is.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7167
