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 23:07:08 PDT 2013 


On 8/12/13 1:45 PM, Xuelei Fan wrote:
> 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".

You break the 1 <= len <= 63 check into two parts, that's why I say 8a 
and 8b.

--Max

>
> 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