On Mon, 14 Nov 2022 21:47:38 GMT, Valerie Peng <valer...@openjdk.org> wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   make class package private
>
> src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 381:
> 
>> 379:         index = indexOf(oid, PKCS9_OIDS, 1);
>> 380:         Class<?> clazz = index == -1 ? BYTE_ARRAY_CLASS: 
>> VALUE_CLASSES[index];
>> 381:         if (clazz == null || !clazz.isInstance(value)) {
> 
> If my reading of the current impl is correct, if clazz is null, the attribute 
> is not supported. The error message seems a bit misleading as it's not really 
> due to the value itself, but the attribute is not supported. Is it just for 
> avoiding NPE and changing it into IAE?

Yes, just to avoid NPE. I can see the message about type is null looks strange. 
I'll change it to something normal.

> test/jdk/sun/security/pkcs/pkcs9/PKCS9AttrTypeTests.java line 176:
> 
>> 174:                 // Encoding is supported
>> 175:                 DerOutputStream dos = new DerOutputStream();
>> 176:                 p9Attr.encode(dos);
> 
> Should we check the encoding has the expected value? Otherwise, it looks like 
> we only require that no exception is thrown?

You mean comparing it with the original value? I tried that but not always the 
same. For example, a string might be an IA5String at the beginning but becomes 
a UTF8String after re-encoding.

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

PR: https://git.openjdk.org/jdk/pull/11070

Reply via email to