Hi Dmitry

On 11/13/2012 08:18 PM, Dmitry Samersoff wrote:
Weijun,

1.
That's line 288. Are you suggesting that port string can be
non-numeric and need a check?

Port could be not a default kerberos port but another one - e.g.
customer can setup kerberos on port *1* if they want.

Yes, but with line 288, hostport will be "host:1". Isn't that expected?


Customer allowed to use service name here instead of numeric port number
if my memory is not bogus.

rfc2782 [1] says it can be only a number:

   Port
        The port on this target host of this service.  The range is 0-
        65535.  This is a 16 bit unsigned integer in network byte order.
        This is often as specified in Assigned Numbers but need not be.

If it's really a service name, I might have to ignore it at the moment because I don't know an API to perform getportbyname().


2.
shadow the real one by prepending it to the bootclasspath.
jtreg allows you to change boot classpath in othervm mode

e.g. :

@run main/othervm -Xbootclasspath/a:../classes/serviceability
-XX:+UnlockDiagnosticVMOptions ... ParserTest

Good suggestion, but the class is built into test.classes and test run in scratch. I cannot find a way to get the relative path to the class. I reply on "javac -d ." to output the class into scratch.

Thanks
Max

[1] http://tools.ietf.org/html/rfc2782



-Dmitry

On 2012-11-13 16:07, Weijun Wang wrote:


On 11/13/2012 07:44 PM, Dmitry Samersoff wrote:
Weijun,

Config.java:1162
       This code is unclear to me.
       if srvs[i] could be "" this code could insert extra space in
       the middle of kdcs string.

It should never be empty. KrbServiceLocator.java:281 makes sure it's a
valid DNS SRV record.


       if srvc[i] couldn't be empty we can return null just
       after line 1160  if srvs.length == 0

Yes, we can.


KrbServiceLocator.java:285

      Kerberos could be run on non standard port - rare case but
      AFAIK we don't limit it. So it's better to use parseInt here.

That's line 288. Are you suggesting that port string can be non-numeric
and need a check?


dns.sh:
     Why we need Shell script here?

I cannot use the real NamingManager inside JDK because it will attempt
to access a real DNS server, thus I write my own fake provider and
shadow the real one by prepending it to the bootclasspath.

Thanks
Max



Regards,
-Dmitry



On 2012-11-13 15:05, Weijun Wang wrote:
Ping again.

The webrev contains codes by myself so I need another reviewer.

Thanks
Max


On 11/09/2012 08:38 AM, Weijun Wang wrote:
Hi Severin

I've created an OpenJDK bug and created a new webrev:

      http://cr.openjdk.java.net/~weijun/8002344/webrev.00/

The Config.java change is identical to yours, and I added a small tweak
in KrbServiceLocator, and a quite ugly regression test.

Anyone can review all the changes?

After the code review, I'll push the change to tl/jdk. I don't see an
OpenJDK user id for you at http://db.openjdk.java.net/people, so I add
your name in

     Contributed-by: Severin Gehwolf <[email protected]>

Thanks
Max


On 11/08/2012 11:46 PM, Severin Gehwolf wrote:
Hi Max,

Thanks for the review!

On Wed, 2012-11-07 at 08:52 +0800, Weijun Wang wrote:
The fix looks fine. There is one place it might get enhanced:

            if (value.charAt(j) == ':') {
                kdcs = (value.substring(0, j)).trim();
            }

So this changes a.com:88 to a.com. If the port is really 88, it's OK.
Otherwise, info gets lost. Maybe we can keep the whole string.

I've removed the entire loop which strips the port from the returned
record. Updated webrev is here:

http://jerboaa.fedorapeople.org/bugs/openjdk/2376501/webrev.1/

BTW, are you OK with contributing the fix into OpenJDK main repo?

Yes, of course :) Just let me know what's to be done to get it pushed.

Cheers,
Severin

On 11/06/2012 11:08 PM, Severin Gehwolf wrote:
Hi,

In Config.java, line 1234 in method getKDCFromDNS(String realm)
there is
a loop which discards earlier values of KDCs returned rather than
concatenating them. This results in a behaviour where only one KDC
in a
seemingly random fashion is returned. In fact, the KDC returned
depends
on the order which KrbServiceLocator.getKerberosService(realm,
"_udp")
returns the servers. The correct behaviour should be to return a
String
containing ALL KDCs available via DNS (separated by spaces).

The webrev is here:
http://jerboaa.fedorapeople.org/bugs/openjdk/2376501/webrev/

Comments and suggestions very welcome!

Thanks,
Severin








Reply via email to