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