Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 17:18:50 GMT, Jamil Nimeh wrote: >> I don't want to go on reading the following bytes to find out what the >> intended tag number is, because that somehow shows I do understand the >> encoding _a lot_ but still don't want to support it (well, actually I only >> understand _a little_). There are only 2 kinds of tags: one <= 30 and one >= >> 31. IMHO, the message has already expressed the meaning that we only support >> the 1st one. >> >> An alternative message I can think of is "Unsupported tag byte: 0xBF", but >> it looks too cryptic. > > I think that is fair. If you don't want to read ahead like that, what about > using the "offset" or "pos" field to give a message like "Tag number over 30 > at offset NN is not supported" (something like that, at least) Maybe don't > worry about the tag value itself, but at least the position in the data > stream. Just a suggestion only, no strong feelings about this either way. There _is_ an `offset` value here but I have really no idea if the user knows where to count from. If we say "offset" then we probably need to tell what data block we are talking about. What if the DerValue is just a portion of a bigger data block? That said, if you really like it, I can add an offset like "tag byte at offset ". I just hope the user can find it. - PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 16:59:54 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/sun/security/util/DerValue.java line 225:
>>
>>> 223: DerValue(byte tag, byte[] buffer, int start, int end, boolean
>>> allowBER) {
>>> 224: if ((tag & 0x1f) == 0x1f) {
>>> 225: throw new IllegalArgumentException("Tag number 31 is not
>>> supported");
>>
>> As number 31 just means the tag is bigger than 31, Is it more accuracy by
>> using "Tag number over 30 is not supported"?
>
> Well, it's a little delicate here. Even if we support multi-byte tag one day,
> this constructor will still only be used to create a single-byte tag
> `DerValue`, and it's illegal for a single byte tag to end with 0x1f. So the
> words above is to remind people that they cannot create a tag number 31
> `DerValue` just because it seems it still fits into the 5 bits. Precisely,
> the words should be "this constructor only supports tag number between 0 and
> 30", but... I'll choose your words.
It makes sense. Your words is good to me.
-
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 17:10:13 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/sun/security/util/DerValue.java line 322:
>>
>>> 320: tag = buf[pos++];
>>> 321: if ((tag & 0x1f) == 0x1f) {
>>> 322: throw new IOException("Tag number over 30 is not
>>> supported");
>>
>> Would it be useful for these types of exception messages to either display
>> the offending tag value or perhaps the tag offset? Just thinking it might
>> be a nice thing for the recipient to know where in the DER encoding the
>> issue is.
>
> I don't want to go on reading the following bytes to find out what the
> intended tag number is, because that somehow shows I do understand the
> encoding _a lot_ but still don't want to support it (well, actually I only
> understand _a little_). There are only 2 kinds of tags: one <= 30 and one >=
> 31. IMHO, the message has already expressed the meaning that we only support
> the 1st one.
>
> An alternative message I can think of is "Unsupported tag byte: 0xBF", but it
> looks too cryptic.
I think that is fair. If you don't want to read ahead like that, what about
using the "offset" or "pos" field to give a message like "Tag number over 30 at
offset NN is not supported" (something like that, at least) Maybe don't worry
about the tag value itself, but at least the position in the data stream. Just
a suggestion only, no strong feelings about this either way.
-
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 16:58:24 GMT, Jamil Nimeh wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update exception wordings
>
> src/java.base/share/classes/sun/security/util/DerValue.java line 322:
>
>> 320: tag = buf[pos++];
>> 321: if ((tag & 0x1f) == 0x1f) {
>> 322: throw new IOException("Tag number over 30 is not
>> supported");
>
> Would it be useful for these types of exception messages to either display
> the offending tag value or perhaps the tag offset? Just thinking it might be
> a nice thing for the recipient to know where in the DER encoding the issue is.
I don't want to go on reading the following bytes to find out what the intended
tag number is, because that somehow shows I do understand the encoding _a lot_
but still don't want to support it (well, actually I only understand _a
little_). There are only 2 kinds of tags: one <= 30 and one >= 31. IMHO, the
message has already expressed the meaning that we only support the 1st one.
An alternative message I can think of is "Unsupported tag byte: 0xBF", but it
looks too cryptic.
-
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 15:53:10 GMT, Xue-Lei Andrew Fan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update exception wordings
>
> src/java.base/share/classes/sun/security/util/DerValue.java line 225:
>
>> 223: DerValue(byte tag, byte[] buffer, int start, int end, boolean
>> allowBER) {
>> 224: if ((tag & 0x1f) == 0x1f) {
>> 225: throw new IllegalArgumentException("Tag number 31 is not
>> supported");
>
> As number 31 just means the tag is bigger than 31, Is it more accuracy by
> using "Tag number over 30 is not supported"?
Well, it's a little delicate here. Even if we support multi-byte tag one day,
this constructor will still only be used to create a single-byte tag
`DerValue`, and it's illegal for a single byte tag to end with 0x1f. So the
words above is to remind people that they cannot create a tag number 31
`DerValue` just because it seems it still fits into the 5 bits. Precisely, the
words should be "this constructor only supports tag number between 0 and 30",
but... I'll choose your words.
-
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 13:57:37 GMT, Weijun Wang wrote:
>> This code change does not intend to support multiple byte tags. Instead, it
>> aims to fail more gracefully when such a tag is encountered. For `DerValue`
>> constructors from an encoding (type I), an `IOException` will be thrown
>> since it's already in the throws clause. For constructors from tag and value
>> (type II), an `IllegalArgumentException` will be thrown. All existing type
>> II callers inside JDK use tag numbers smaller than 31.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update exception wordings
src/java.base/share/classes/sun/security/util/DerValue.java line 322:
> 320: tag = buf[pos++];
> 321: if ((tag & 0x1f) == 0x1f) {
> 322: throw new IOException("Tag number over 30 is not supported");
Would it be useful for these types of exception messages to either display the
offending tag value or perhaps the tag offset? Just thinking it might be a
nice thing for the recipient to know where in the DER encoding the issue is.
-
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
On Thu, 8 Apr 2021 13:57:37 GMT, Weijun Wang wrote:
>> This code change does not intend to support multiple byte tags. Instead, it
>> aims to fail more gracefully when such a tag is encountered. For `DerValue`
>> constructors from an encoding (type I), an `IOException` will be thrown
>> since it's already in the throws clause. For constructors from tag and value
>> (type II), an `IllegalArgumentException` will be thrown. All existing type
>> II callers inside JDK use tag numbers smaller than 31.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update exception wordings
Looks good to me, except a minor comment.
src/java.base/share/classes/sun/security/util/DerValue.java line 225:
> 223: DerValue(byte tag, byte[] buffer, int start, int end, boolean
> allowBER) {
> 224: if ((tag & 0x1f) == 0x1f) {
> 225: throw new IllegalArgumentException("Tag number 31 is not
> supported");
As number 31 just means the tag is bigger than 31, Is it more accuracy by using
"Tag number over 30 is not supported"?
-
Marked as reviewed by xuelei (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v3]
> This code change does not intend to support multiple byte tags. Instead, it > aims to fail more gracefully when such a tag is encountered. For `DerValue` > constructors from an encoding (type I), an `IOException` will be thrown since > it's already in the throws clause. For constructors from tag and value (type > II), an `IllegalArgumentException` will be thrown. All existing type II > callers inside JDK use tag numbers smaller than 31. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: update exception wordings - Changes: - all: https://git.openjdk.java.net/jdk/pull/3391/files - new: https://git.openjdk.java.net/jdk/pull/3391/files/46b3700b..9b0f9db9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3391=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3391=01-02 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3391.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391 PR: https://git.openjdk.java.net/jdk/pull/3391
