Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot

Xuelei Fan xuelei.fan at oracle.com
Sun Aug 11 22:45:12 PDT 2013 

new webrev: http://cr.openjdk.java.net/~xuelei/8020842/webrev.06/

> Lines 280 and 333: How about we call them steps 8a and 8b?

Step 8 is referring to the steps in RFC 3490.  Let's use "step 8".

Thanks,
Xuelei

On 8/12/2013 11:11 AM, Weijun Wang wrote:
> 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