Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot
Weijun Wang
weijun.wang at oracle.com
Sun Aug 11 20:11:42 PDT 2013
I think the fix is adequate and necessary.
One problem: lines 367-373 adds a new IAE to ToUnicode but the method
should not fail forever.
And some small comments on styles etc.
On 8/12/13 9:09 AM, Xuelei Fan wrote:
> new webrev: http://cr.openjdk.java.net/~xuelei/8020842/webrev.05/
Lines 123 and 185:
184 p = q + 1;
185 if (p < input.length() || q == (input.length() - 1)) {
186 // has more labels, or keep the trailing dot as at
present
187 out.append('.');
188 }
I prefer
if (q < input.length()) { // Ah, a dot!
out.append('.');
}
p = q + 1;
Lines 282, 335, 270: Insert a blank after "if".
Lines 284 and 372: nslookup uses "empty label", which I like better.
Lines 453 and 460: Personally I don't like the parenthesis for the whole
return value, but you have your choice.
Lines 280 and 333: How about we call them steps 8a and 8b?
>
> Added a new test to test illegal hostname in SNIHostName.
Excellent. Otherwise I will be wondering why the fix in IDN could solve
the problem as described in the bug description.
Thanks
Max
>
> Xuelei
>
> On 8/10/2013 10:49 AM, Xuelei Fan wrote:
>> Hi Michael,
>>
>> It is pretty hard to get the issue solved in SNIHostName in a good
>> sharp. Here is my try to state why we should fix the issue in IDN.
>>
>> In SNIHostName, the following hostname are not accepted as valid hostname:
>> 1. empty hostname
>> 2. hostname ends with a trailing dot
>> 3. hostname does not comply to RFC 3490.
>>
>> The process in SNIHostName looks like:
>> 1. call IDN.toASCII() to convert a string hostname
>> 2. check that the return value of #1 is an valid hostname (non-empty,
>> non-end-with-tailing-dot).
>>
>> At present, the IDN cannot handle the following IDN properly.
>> 1. returns "com" for "com."
>> the trailing dot is swallowed.
>>
>> 2. throws StringIndexOutOfBoundsException for "."
>> If "." is an valid IDN that comply to RFC 3490, IDN.toASCII() should
>> be able to handle it; otherwise, IDN.toASCII() should throw IAE as the
>> specification suggested. However, IDN.toASCII(".") throws
>> StringIndexOutOfBoundsException, this behavior does not comply the the
>> specification:
>>
>> 3. throws StringIndexOutOfBoundsException for "example...net"
>> As #2.
>>
>> We can address #1 and #2 in SNIHostName, but the checking is overloaded
>> as IDN also need to address the issue. And SNIHostName has to know
>> what's the separators (".", "\u3002, etc) of IDN in order to check the
>> dot character. It is not a good encapsulation, and involved in too much
>> about the details of domain name, I think.
>>
>> It is a little big hard to address #3 in SNIHostName.
>>
>> Both all of above issue can be easily addressed in IDN. And once IDN
>> addressed these issues, the current SNIHostName is able to handle
>> invalid hostname (empty, trailing dot, etc) correctly. We won't need to
>> touch SNIHostName any more.
>>
>> Please consider it.
>>
>> The latest webrev is at:
>> http://cr.openjdk.java.net/~xuelei/8020842/webrev.02/
>>
>> Thanks,
>> Xuelei
>>
>> On 8/10/2013 9:13 AM, Xuelei Fan wrote:
>>> Hi Michael,
>>>
>>> I plan to address this issue in SNIHostName. I have filled another two
>>> the potential bugs in IDN.
>>>
>>> Thank you, and other people, for the feedback.
>>>
>>> Thanks,
>>> Xuelei
>>>
>>> On 8/9/2013 11:25 PM, Xuelei Fan wrote:
>>>> On 8/9/2013 7:31 PM, Michael McMahon wrote:
>>>>> I don't see how this fixes the original problem as the SNIHostName spec
>>>>> still doesn't like hostnames with a trailing '.'
>>>>>
>>>> The bug description did not reflect the IDN specification correctly. If
>>>> "com." is a valid IDN, SNIHostName should accept it an an valid
>>>> hostname. The host name in SNIHostName is nothing more or less than an
>>>> standard IDN.
>>>>
>>>> I added a comment in the bug: "com." and "." are valid IDN according the
>>>> IDN and domain name specifications. I will contact the bug reporter
>>>> about this point.
>>>>
>>>> Xuelei
>>>>
>>>>> I'd prefer to check first where that requirement is coming from, if it is
>>>>> actually necessary, and if not consider removing it from SNIHostName.
>>>>> If it is necessary, then the check should be implemented in SNIHostName.
>>>>>
>>>>> Michael
>>>>>
>>>>> On 09/08/13 05:28, Xuelei Fan wrote:
>>>>>> Thanks for your feedback and suggestions.
>>>>>>
>>>>>> Here is the new webrev:
>>>>>> http://cr.openjdk.java.net/~xuelei/8020842/webrev.02/
>>>>>>
>>>>>> "." is regarded as valid IDN in this update.
>>>>>>
>>>>>> Thanks,
>>>>>> Xuelei
>>>>>>
>>>>>> On 8/9/2013 10:50 AM, Xuelei Fan wrote:
>>>>>>> On 8/9/2013 10:14 AM, Weijun Wang wrote:
>>>>>>>>
>>>>>>>> On 8/9/13 9:37 AM, Xuelei Fan wrote:
>>>>>>>>> On 8/9/2013 9:22 AM, Weijun Wang wrote:
>>>>>>>>>> I tried nslookup. Those with ".." inside are illegal,
>>>>>>>>>>
>>>>>>>>>> $ nslookup com..
>>>>>>>>>> nslookup: 'com..' is not a legal name (empty label)
>>>>>>>>>>
>>>>>>>>>> but
>>>>>>>>>>
>>>>>>>>>> $ nslookup .
>>>>>>>>>> Server: 192.168.10.1
>>>>>>>>>> Address: 192.168.10.1#53
>>>>>>>>>>
>>>>>>>>>> Non-authoritative answer:
>>>>>>>>>> *** Can't find .: No answer
>>>>>>>>>>
>>>>>>>>> Thanks for the testing. The behaviors are the same as this fix now.
>>>>>>>> No exactly. It seems nslookup still regards "." legal but just cannot
>>>>>>>> find an IP for it.
>>>>>>>>
>>>>>>> I'm not sure whether a root domain name can be stand alone. Root label
>>>>>>> is not considered as a label in IDN. I think it is safe to regard that
>>>>>>> "." is not a valid IDN as it contains no label. Anyway, it is a corner
>>>>>>> case.
>>>>>>>
>>>>>>> There are many online IDN conversion web services, some of them can
>>>>>>> convert ".", some of the cannot. In the present implementation, we
>>>>>>> cannot recognize ".", and IDN.toASCII(".") throws
>>>>>>> StringIndexOutOfBoundsException. With this fix, I was wondering IAE is
>>>>>>> a better exception for IDN.toASCII(".").
>>>>>>>
>>>>>>>>> Learn something new today to use nslookup.
>>>>>>>>>
>>>>>>>>>> Also, since this bug was originally about SNIHostName, do you need to
>>>>>>>>>> add some extra restriction there to reject "oracle.com." things?
>>>>>>>>>>
>>>>>>>>> No, we cannot restrict the format of IDN in SNIHostName more than in
>>>>>>>>> IDN. However, we may need to rethink about the comparing of two
>>>>>>>>> IDN, for
>>>>>>>>> example, "example.com." should equal to "example.com". I want to
>>>>>>>>> consider it in another bug.
>>>>>>>> Not sure. Does the spec say IDN and SNIHostName are equivalent sets?
>>>>>>>> And
>>>>>>>> it's not one is another's subset?
>>>>>>>>
>>>>>>> Per TLS specification, host name in SNI is an IDN. The spec of
>>>>>>> SNIHostname says, "hostname is not a valid Internationalized Domain Name
>>>>>>> (IDN) compliant with the RFC 3490 specification". The spec in
>>>>>>> SNIHostName has the same means as IDN. I won't want to add additional
>>>>>>> restrict beyond the specification of an IDN.
>>>>>>>
>>>>>>> Xuelei
>>>>>>>
>>>>>>>>> Can I push the changeset?
>>>>>>>> I think it's better to ask someone in the networking team to make the
>>>>>>>> suggestion. From what I read Michael in this thread, he does not seem
>>>>>>>> totally agreed with your code changes (at least not the 00 version).
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Max
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Xuelei
>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Max
>>>>>>>>>>
>>>>>>>>>> On 8/9/13 8:41 AM, Xuelei Fan wrote:
>>>>>>>>>>> Ping.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Xuelei
>>>>>>>>>>>
>>>>>>>>>>> On 8/7/2013 11:17 PM, Xuelei Fan wrote:
>>>>>>>>>>>> Please review the new update:
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net./~xuelei/8020842/webrev.01/
>>>>>>>>>>>>
>>>>>>>>>>>> With this update, "com." is valid (return "com."); "." and
>>>>>>>>>>>> "example..com" are invalid. And IAE will be thrown for invalid
>>>>>>>>>>>> IDN.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Xuelei
>>>>>>>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the net-dev
mailing list