Re: SocketPermission's implies() interesting behavior

[Neil Richards]

On 1 March 2011 09:18, Chris Hegarty  wrote:
> Michael,
>
> Can you please take a look at this change, CR 7021280: "SocketPermission
> trustProxy should accept wildcards".
>
> This patch came from Charles (cc'ed), and I agree with the changes. Can you
> please take a look and give your feedback.
>
> http://cr.openjdk.java.net/~chegar/7021280/webrev.00/webrev/
>
> -Chris.

In the suggested fix, I see that the comparison becomes
'thatHost.endsWith(this.cname)' if 'this.wildcard' is 'true'.
Prior to the change, the comparison was (always)
'thisHost.equalsIgnoreCase(thatHost)'.

So, on the surface, it looks like a case-insensitive comparison -
String.equalsIgnoreCase() - has been replaced (when 'this.wildcard' is
'true') with a case-sensitive comparison - String.endsWith().

Is there a reason why this change in case sensitivity is not a problem
in this instance, or does the suggested fix need to be reworked to
make the new comparison (also) case-insensitive ?

- Neil

--
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: SocketPermission's implies() interesting behavior

[Neil Richards]

On 11 March 2011 14:39, Chris Hegarty  wrote:
> As Michael (cc'ed) mentioned in an earlier mail, he is going to be making
> some significant changes in this area in the next week or two. He will
> include this change with the other one, so their impact can be considered
> together, if that is ok?

That's fine by me, so long as the changeset that addresses this
problem is linked back to this conversation (so that one can track the
progress of the fix for this problem into the component, then master
repository).

Thanks for helping with this,
Neil

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: SocketPermission's implies() interesting behavior

[Neil Richards]

On 10 March 2011 03:18, Charles Lee  wrote:
> With a quick search in the SocketPermission using "cname =" as search word,
> cname always seems to be lower cases. But hostname does not. It may need
> some rework on the patch.

I think using String.regionMatches() [1] for the comparison (with
'ignoreCase' set to 'true') would solve the case-sensitivity problem.

Please find attached an updated diff with how that would look.
(Apols for not inlining it, I just know my current mail client would
mangle it with line wrapping).


> And more, every place where cname compare with another cname should use
> normal equalsTo (equalsIgnoreCase is not needed). Every place where getName
> and hostname is involved should use case-insensitive comparison. Any
> opinion, Neil and Chris?

>From a quick glance at the code, I think you're correct.
However, I'd suggest making those improvements under a separate RFE,
as they wouldn't directly relate to CR 7021280.

Hope this helps,
Neil

[1] 
http://download.oracle.com/javase/6/docs/api/java/lang/String.html#regionMatches%28boolean,%20int,%20java.lang.String,%20int,%20int%29

--
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
diff --git a/src/share/classes/java/net/SocketPermission.java b/src/share/classes/java/net/SocketPermission.java
--- a/src/share/classes/java/net/SocketPermission.java
+++ b/src/share/classes/java/net/SocketPermission.java
@@ -815,11 +815,15 @@
 String thisHost = hostname;
 String thatHost = that.hostname;
 
-if (thisHost == null)
+if (thisHost == null) {
 return false;
-else
+} else if (wildcard) {
+final int cnameLength = cname.length();
+return thatHost.regionMatches(true, 
+(thatHost.length() - cnameLength), cname, 0, cnameLength);
+} else {
 return thisHost.equalsIgnoreCase(thatHost);
-
+} 
 }
 /**
  * Checks two SocketPermission objects for equality.

Re: SocketPermission's implies() interesting behavior

[Neil Richards]

On Wed, 2011-03-16 at 10:43 +, Neil Richards wrote:
> On 11 March 2011 14:39, Chris Hegarty  wrote:
> > As Michael (cc'ed) mentioned in an earlier mail, he is going to be making
> > some significant changes in this area in the next week or two. He will
> > include this change with the other one, so their impact can be considered
> > together, if that is ok?
> 
> That's fine by me, so long as the changeset that addresses this
> problem is linked back to this conversation (so that one can track the
> progress of the fix for this problem into the component, then master
> repository).

Can you tell me if the changes headlined above have yet been made to the
code (and point me to the URL of the change) ?

Thanks,
Neil

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: JDK 8 Code review: 7073491

[Neil Richards]

On Thu, 2011-09-15 at 13:46 +0100, Michael McMahon wrote:
> Hi,
> 
> Could I get the following code change reviewed please?
> 
> The problem is in AbstractPlainDatagramSocket.create(). If 
> ResourceManager.beforeUdpCreate()
> throws an exception then fd is left set in the impl object. And if the 
> finalizer for this object
> runs then it will attempt to close the object and decrement the counter, 
> thus double counting the close.
> It only happens via the finalizer because the impl is not returned to 
> the application because the
> original exception is thrown from a DatagramSocket constructor.
> 
> http://cr.openjdk.java.net/~michaelm/7073491%3a/
> 
> Thanks
> Michael.
> 

This looks good to me.
- Neil

hg: jdk8/tl/jdk: 2 new changesets

[neil . richards]

Changeset: dd55467dd1f2
Author:ngmr
Date:  2011-10-10 14:50 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/dd55467dd1f2

7099119: Remove unused dlinfo local variable in launcher code
Reviewed-by: ohair, chegar, ngmr
Contributed-by: Steve Poole 

! src/solaris/bin/java_md.c

Changeset: 5f336e0d4d97
Author:ngmr
Date:  2011-10-10 16:13 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/5f336e0d4d97

Merge

Re: java.net.NetworkInterface.getNetworkInterfaces does not work properly on AIX with IPv6

[Neil Richards]

On Thu, 2011-10-20 at 17:12 +0800, Jonathan Lu wrote:
> On 10/20/2011 02:35 PM, Steve Poole wrote:
> Thanks Steve, I've updated the test case and patch, see the attachments.
> I've added IBM portions copyright comment to both headers.
> 
> - Jonathan

For ease of review, I've uploaded the suggested fix as a webrev [1].

Whilst creating this, I stripped the (large amount of) extraneous
trailing whitespace characters from the lines added by the patch.
(It would be helpful to check for this when posting patches).


Looking at the change, I have some concerns.

Firstly, the added AIX-specific code (between lines 1102-1187) has been
placed in a block of code that looks to be Linux-specific (lines
1013-1365, protected by '#ifdef __linux__').

So it's unclear why any of this code will be used by AIX.


Secondly, if an exception occurs whilst the list of interfaces is being
compiled, both Linux and Solaris look to return the list compiled up to
the point of the exception (blocks starting at lines 1222 & 1505).

In the suggested AIX code, however, the compiled interface list is freed
up in this situation, and NULL returned (block starting at line 1174).

It seems to me that these routines should present similar semantics
(across the 3 platforms).
As the interface list returned is an augmentation of an interface list
that is originally given to the function (the original value of 'ifs'),
the suggested AIX code may free off entries in the list that it was not
responsible for allocating, which runs the risk of resulting in
duplicate calls to free().

Could you please investigate where the responsibilities properly lie for
allocation and deallocation of the entries in the interface list given
to the enumIPv6Interfaces() function, then update the proposal /
comments appropriately?


Also, I'm concerned that the name of the testcase provided is overly
vague (what _specifically_ about NetworkInterface does it test?), and
that the @summary description does not describe what the intention of
the test is.
In particular, there is nothing platform-specific about the testcase, so
I would not expect its description to be in terms of something
AIX-specific.


Hope this helps,
Regards, Neil

[1] http://cr.openjdk.java.net/~ngmr/ojdk-172/webrev.00/

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

hg: jdk8/tl/jdk: 7100054: (porting) Native code should include fcntl.h and unistd.h rather than sys/fcntl.h and sys/unistd.h

[neil . richards]

Changeset: c3da0672a882
Author:ngmr
Date:  2011-10-13 12:30 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c3da0672a882

7100054: (porting) Native code should include fcntl.h and unistd.h rather than 
sys/fcntl.h and sys/unistd.h
Summary: Use POSIX defined includes for unistd.h and fcntl.h
Reviewed-by: dholmes, alanb, chegar, ngmr
Contributed-by: Charles Lee 

! src/solaris/native/sun/nio/fs/genSolarisConstants.c
! src/solaris/native/sun/nio/fs/genUnixConstants.c

Re: Question about getaddrinfo in Inet4AddressImpl.c

[Neil Richards]

On Wed, 2011-11-02 at 23:07 +0800, Charles Lee wrote:
> On 10/26/2011 06:31 PM, Chris Hegarty wrote:
> > On 26/10/2011 10:36, Alan Bateman wrote:
> >> On 26/10/2011 10:24, Charles Lee wrote:
> >>>
> >>>
> >>> />>> I don't think this code has changed too much since then and
> >>> probably could do with a clean-up./
> >>> Not true.
> >> I'm talking about the InetAddress* code, that hasn't changed
> >> significantly and probably could do with some modernization now.
> >
> > Yes, please submit a patch for this cleanup and we'll review it.
> >
> > -Chris.
> >
> >>
> >> -Alan.
> >
> Hi Chris, Alan and Neil,
> 
> Here is the rest of the patch (attached). The patch is a little big 
> because some formats.
> 

For ease of review, I've uploaded this change (ie. the combination of
Charles' 'patch.contr.v1' and 'patch.contr.part2') as a webrev [1].

My initial (and picky) observation is that the code indentation needs a
little work to comply with the OpenJDK coding conventions [2].
(Particularly using a standard indentation for 4 spaces).

Regards, Neil 

[1] http://cr.openjdk.java.net/~ngmr/ojdk-229/webrev.00/
[2] http://www.oracle.com/technetwork/java/codeconventions-150003.pdf

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: Question about getaddrinfo in Inet4AddressImpl.c

[Neil Richards]

On Wed, 2011-11-09 at 12:19 +0800, Charles Lee wrote:
> On 11/09/2011 03:25 AM, Chris Hegarty wrote:
> > Charles,
> >
> > Is it possible to fix up the style issues, etc that Neil pointed out, 
> > and have the webrev updated?
> >

> Hi Chris,
> 
> Here it is. (attached)
> 

And here it is, in webrev form [1].

Regards, Neil

[1] http://cr.openjdk.java.net/~ngmr/ojdk-229/webrev.01/

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: Question about getaddrinfo in Inet4AddressImpl.c

[Neil Richards]

On Tue, 2011-11-15 at 19:37 +0800, Charles Lee wrote:
> On 11/15/2011 07:10 PM, Chris Hegarty wrote:
> > On 11/15/11 06:57 AM, Charles Lee wrote:
> >> 
> 
> >>>
> >> Sigh. Chris, I still fail to see those warnings, even if I do a very
> >> clean remove of my build directory and rebuild the whole jdk 
> >
> > The warning are being generated on Solaris when using the Sun 
> > compilers ( both full and incremental builds ).
> >
> >> Here is the new (big) patch(attached) according to the comments from you
> >> and Mike. Thank you guys :-)
> >
> > I can't apply the patch you sent. Is it possible to generate a webrev, 
> > another patch, or let me know how to apply the the one you sent?
> >
> > Thanks,
> > -Chris.
> >
> Hi Chris,
> 
> patch -p0 < mypatch under the jdk directory will do the trick.
> 
> I will try to find a solaris to test my patch.
> 

Here is Charles' latest patch in webrev form [1].

Regards, Neil

[1] http://cr.openjdk.java.net/~ngmr/ojdk-229/webrev.02/

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

hg: jdk8/tl/jdk: 7112670: Inet4AddressImpl should use getaddrinfo/getnameinfo

[neil . richards]

Changeset: 81987765cb81
Author:ngmr
Date:  2011-11-11 14:40 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/81987765cb81

7112670: Inet4AddressImpl should use getaddrinfo/getnameinfo
Reviewed-by: chegar, alanb, mduigou, ngmr
Contributed-by: Charles Lee 

! src/solaris/native/java/net/Inet4AddressImpl.c
! src/solaris/native/java/net/Inet6AddressImpl.c
! src/solaris/native/java/net/net_util_md.c
! src/solaris/native/java/net/net_util_md.h

Re: Question about getaddrinfo in Inet4AddressImpl.c

[Neil Richards]

On Wed, 2011-11-16 at 20:57 +, Chris Hegarty wrote:
> Thank you Charles, and Neil.
> 
> I ran some builds and tests and all looks good to me. Thanks for fixing 
> the warnings, I know they were not caused by your changes.
> 
> Just noticed that we haven't just filed a bug for this, so I just created:
>CR 7112670: Inet4AddressImpl should use getaddrinfo/getnameinfo
> 
> Neil,
>I assume you will push this?
> 
> -Chris.
> 

Hi Chris,
Thank you for creating the bug id for this issue.

I've now pushed the change [1].

However, I've just realised I've foolishly pushed a previous version of
the change, and not the final agreed version. (aargh!)

Could you please advise me how i might best rectify this, and furnish me
with another bug id if I need one for this purpose?

Humble apologies for this :-(

Regards, Neil

[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/81987765cb81

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: Question about getaddrinfo in Inet4AddressImpl.c

[Neil Richards]

On Tue, 2011-11-22 at 09:38 +, Chris Hegarty wrote:
> Let me know the details synopsis/descriptions/etc and I'll file a new 
> CR. I'm guessing its just some cleanup/style issues, right?
> 
> -Chris.

I've uploaded a webrev with the gap between the two [1].

The differences are the use of memset (rather than bzero) and NI_MAXHOST
(rather than MAXHOSTNAMELEN).

Thanks for helping me out with this,

Regards, Neil

[1] http://cr.openjdk.java.net/~ngmr/ojdk-229.1/webrev.00/

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

hg: jdk8/tl/jdk: 7114558: Inet4AddressImpl should use memset (rather than bzero) and NI_MAXHOST (rather than MAXHOSTNAMELEN)

[neil . richards]

Changeset: ee2fa62fb09f
Author:ngmr
Date:  2011-11-22 09:51 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ee2fa62fb09f

7114558: Inet4AddressImpl should use memset (rather than bzero) and NI_MAXHOST 
(rather than MAXHOSTNAMELEN)
Reviewed-by: chegar
Contributed-by: Neil Richards 

! src/solaris/native/java/net/Inet4AddressImpl.c
! src/solaris/native/java/net/Inet6AddressImpl.c

Re: Question about getaddrinfo in Inet4AddressImpl.c

[Neil Richards]

On Tue, 2011-11-22 at 10:49 +, Chris Hegarty wrote:
> Neil,
> 
> I filed:
> CR 7114558: "Inet4AddressImpl should use memset (rather than bzero) and 
> NI_MAXHOST (rather than MAXHOSTNAMELEN)"
> 
> And also reviewed your webrev. Looks fine.
> 
> -Chris.

Hi Chris,
Thanks for reviewing this for me - I've now pushed the change up [1].

Regards, Neil

[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ee2fa62fb09f

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

hg: jdk8/tl/jdk: 7115070: (fs) lookupPrincipalByName/lookupPrincipalByGroupName should treat ESRCH as not found

[neil . richards]

Changeset: 955aae8c1106
Author:ngmr
Date:  2011-11-24 11:34 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/955aae8c1106

7115070: (fs) lookupPrincipalByName/lookupPrincipalByGroupName should treat 
ESRCH as not found
Reviewed-by: alanb
Contributed-by: Jonathan Lu 

! src/solaris/native/sun/nio/fs/UnixNativeDispatcher.c

hg: jdk8/tl/jdk: 7094995: Trailing daemon thread causes continuous GC in agentvm mode

[neil . richards]

Changeset: 6fbd69f8e3ab
Author:ngmr
Date:  2011-11-18 09:03 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6fbd69f8e3ab

7094995: Trailing daemon thread causes continuous GC in agentvm mode
Summary: Shutdown GcInducingThread once test (successfully) finishes
Reviewed-by: alanb, chegar, dholmes, darcy
Contributed-by: Neil Richards 

! test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java

hg: jdk8/tl/jdk: 7118907: InetAddress.isReachable() should return false if sendto fails with EHOSTUNREACH

[neil . richards]

Changeset: c508f38245f8
Author:ngmr
Date:  2011-12-12 11:41 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c508f38245f8

7118907: InetAddress.isReachable() should return false if sendto fails with 
EHOSTUNREACH
Reviewed-by: alanb, chegar
Contributed-by: Charles Lee 

! src/solaris/native/java/net/Inet4AddressImpl.c
! src/solaris/native/java/net/Inet6AddressImpl.c

Re: EHOSTUNREACH should not be considered as an exception when send on a loopback network interface.

[Neil Richards]

On Thu, 2011-12-08 at 10:54 +, Chris Hegarty wrote:
> On 08/12/2011 03:16, Charles Lee wrote:
> > 
> > Hi Alan, hi Chris,
> >
> > Thank you for reviewing this and creating a bug id for me. I have put a
> > revised webrev at http://cr.openjdk.java.net/~littlee/7118907/webrev.00/
> >  .
> 
> Looks fine to me.
> 
> Let me know if you need help with the push.
> 
> -Chris.
> 
> > If it is ok for you guys, maybe Neil can help :-)
> >

I've now pushed this change up to jdk8/tl/jdk [1].

Regards,
Neil

[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c508f38245f8

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

hg: jdk8/tl/jdk: 7123229: (coll) EnumMap.containsValue(null) returns true

[neil . richards]

Changeset: 863a20b0bf08
Author:ngmr
Date:  2012-01-10 00:07 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/863a20b0bf08

7123229: (coll) EnumMap.containsValue(null) returns true
Summary: java.util.EnumMap.NULL equals() must only be true for itself
Reviewed-by: alanb, mduigou
Contributed-by: Neil Richards 

! src/share/classes/java/util/EnumMap.java
+ test/java/util/EnumMap/UniqueNullValue.java

hg: jdk8/tl/jdk: 7133301: (process) UNIXProcess_md.c should include sys/wait.h rather than wait.h

[neil . richards]

Changeset: 13978750cb87
Author:ngmr
Date:  2012-01-31 10:31 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/13978750cb87

7133301: (process) UNIXProcess_md.c should include sys/wait.h rather than wait.h
Reviewed-by: alanb
Contributed-by: Jonathan Lu 

! src/solaris/native/java/lang/UNIXProcess_md.c

Re: GC closes my ServerSocket

[Neil Richards]

On Wed, 2012-06-13 at 17:23 +0800, Weijun Wang wrote:
> Please anyone take a review:
> 
> http://cr.openjdk.java.net/~weijun/7176574/webrev.00/
> 
> By assigning to a local variable hopefully it stays alive on the stack 
> during the whole method.
> 
> Noreg-self.
> 
> *Chris*: I didn't indented the whole test by wrapping them into a 
> try-finally (or try-with-resources) block. The test runs in othervm and 
> I guess the sockets will be closed anyway.
> 
> Thanks
> Max
> 
> On 06/13/2012 05:08 PM, Chris Hegarty wrote:
> >
> >
> > On 13/06/2012 09:51, Alan Bateman wrote:
> >> On 13/06/2012 09:38, Weijun Wang wrote:
> >>> Hi All
> >>>
> >>> I have a test that basically looks like:
> >>>
> >>> int p = new ServerSocket(0).getLocalPort();
> >>> //
> >>> new Socket("localhost", p);
> >>>
> >>> Recently it's failing on solaris-i586, and after some investigation, I
> >>> realize that the ServerSocket object is GC'ed and auto-closed.
> >>>
> >>> (But why only recently?)
> >>>
> >>> So I change the first line to
> >>>
> >>> ServerSocket ss = new ServerSocket(0);
> >>> int p = ss.getLocalPort();
> >>>
> >>> and it's running fine.
> >>>
> >>> I want to know if the ServerSocket object still has a chance to be
> >>> closed. If yes, I'll add a
> >>>
> >>> ss.close();
> >>>
> >>> at the end to be safer.
> >>>
> >>> Thanks
> >>> Max
> >> HotSpot changes I assume, perhaps changes to the reference processing or
> >> default heap settings.
> >
> > Right, I assume there was some VM change that started this test to fail
> > recently, but clearly this is a test issue. It was just passing all this
> > time by accident, and there is an inherent race between the test and the
> > GC/finalizer thread.
> >
> > You should fix the test as you suggested. Also close the serversocket in
> > a finally block ( or equivalent ). You should not rely on the finalizer
> > to close it out.
> >
> > -Chris.
> >
> >>
> >> -Alan

Would this be a good candidate to use Automatic Resource Management ?

i.e. instead of doing this:

ServerSocket ss1 = null;
ServerSocket ss2 = null;
try {
ss1 = new ServerSocket(0);
ss2 = new ServerSocket(0);
int p1 = ss1.getLocalPort();
int p2 = ss1.getLocalPort();
//...
} finally {
if (ss1 != null) ss1.close();
if (ss2 != null) ss2.close();
}

one would do this:

try (ServerSocket ss1 = new ServerSocket(0);
ServerSocket ss2 = new ServerSocket(0)) {
int p1 = ss1.getLocalPort();
int p2 = ss1.getLocalPort();
//...
}

Regards,
Neil

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: Code Review Request: 6953455 CookieStore.add() cannot handle null URI parameter, contrary to the API specification

[Neil Richards]

On Fri, 2011-09-30 at 10:08 -0700, Kurchi Hazra wrote:
> 
> 
> Hi,
> 
> The CookieStore.add() method throws a Null Pointer Exception when
> null is passed as the uri parameter, although this is allowed
> according to the method spec. 
> 
>  The exception is thrown because uri.getHost() is called on a null
> uri in an effort to add it to the uriIndex, one of the hash maps
> constituting the CookieStore. The fix would be to simply bypass adding
> the cookie to the uriIndex when uri is null.
>
> 
> The fix involves updates in: 
> src/share/classes/java/net/InMemoryCookieStore.java
> 
> Webrev : http://cr.openjdk.java.net/~chegar/6953455/webrev.00/webrev/ 
> 
> Thanks,
> Kurchi
> 

Hi,
This bug fix looks to have been well bedded into the openjdk 8 code
stream at this point.

Would this be a good item to be applied to the jdk7u code stream ?

Regards,
Neil

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: Code Review Request: 6953455 CookieStore.add() cannot handle null URI parameter, contrary to the API specification

[Neil Richards]

Hi Chris,
Some QA folk round these parts observed the problem in 7.

I told them of the fix in 8 [1] & they expressed interest in it being
backported to 7u.

I've confirmed that the problem still exists in the jdk7u code.

I've uploaded a webrev of the fix applied back onto jdk7u-dev [2], for
review.

If approved, I could push the change to jdk7u-dev too, unless that's
considered to be stepping on Kurchi's toes or otherwise bad form.

Regards,
Neil

[1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/74f5fef1d961
[2] http://cr.openjdk.java.net/~ngmr/6953455.7u/webrev.00/

On Wed, 2012-07-04 at 21:34 +0100, Chris Hegarty wrote:
> Seems like a reasonable candidate for a backport. Are you encountering it in 
> 7?
> 
> -Chris
> 
> On 4 Jul 2012, at 15:19, Neil Richards  wrote:
> 
> > On Fri, 2011-09-30 at 10:08 -0700, Kurchi Hazra wrote:
> >> 
> >> 
> >> Hi,
> >> 
> >>The CookieStore.add() method throws a Null Pointer Exception when
> >> null is passed as the uri parameter, although this is allowed
> >> according to the method spec. 
> >> 
> >> The exception is thrown because uri.getHost() is called on a null
> >> uri in an effort to add it to the uriIndex, one of the hash maps
> >> constituting the CookieStore. The fix would be to simply bypass adding
> >> the cookie to the uriIndex when uri is null.
> >> 
> >> 
> >> The fix involves updates in: 
> >> src/share/classes/java/net/InMemoryCookieStore.java
> >> 
> >> Webrev : http://cr.openjdk.java.net/~chegar/6953455/webrev.00/webrev/ 
> >> 
> >> Thanks,
> >> Kurchi
> >> 
> > 
> > Hi,
> > This bug fix looks to have been well bedded into the openjdk 8 code
> > stream at this point.
> > 
> > Would this be a good item to be applied to the jdk7u code stream ?
> > 
> > Regards,
> > Neil
> > 
> > -- 
> > Unless stated above:
> > IBM email: neil_richards at uk.ibm.com
> > IBM United Kingdom Limited - Registered in England and Wales with number 
> > 741598.
> > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
> > 


-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

hg: jdk8/tl/jdk: 8000955: Hashtable.Entry.hashCode() does not conform to Map.Entry.hashCode() defined behaviour

[neil . richards]

Changeset: 5eed4a92ca8c
Author:ngmr
Date:  2012-10-17 13:35 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/5eed4a92ca8c

8000955: Hashtable.Entry.hashCode() does not conform to Map.Entry.hashCode() 
defined behaviour
Reviewed-by: mduigou, alanb

! src/share/classes/java/util/Hashtable.java
+ test/java/util/Map/EntryHashCode.java