Re: [cp-patches] [RFC/PATCH] Fix NPE in HashMap.put()
On Mon, 2011-10-24 at 10:34 +0200, Mark Wielaard wrote:
> On Mon, 2011-10-24 at 10:24 +0200, Mark Wielaard wrote:
> > On Mon, 2011-10-24 at 10:11 +0300, Pekka Enberg wrote:
> > > @@ -345,7 +345,10 @@ public class HashMap extends AbstractMap
> > >
> > > while (e != null)
> > >{
> > > -if ((key.hashCode() == e.key.hashCode()) && equals(key, e.key))
> > > +int hash1 = key == null ? 0 : key.hashCode();
> > > +int hash2 = e.key == null ? 0 : e.key.hashCode();
> > > +
> > > +if ((hash1 == hash2) && equals(key, e.key))
> > >{
> > > e.access(); // Must call this for bookkeeping in
> > > LinkedHashMap.
> > > V r = e.value;
> >
> > Are you sure that is right?
> > What about a key which isn't null but has a hashCode() of zero?
>
> Doh. That wouldn't be a problem, because that would still trigger the
> equals(key, e.key) to be called... OK, I think this is the right fix.
Actually, one suggestion for a micro-optimization. The compiler might
detect that hash1 only needs to be calculated once, but we can make that
explicit by pushing the int hash1 = ...; line just before the while
loop.
Thanks,
Mark
Re: [cp-patches] [RFC/PATCH] Fix NPE in HashMap.put()
On Mon, Oct 24, 2011 at 11:24 AM, Mark Wielaard wrote:
> On Mon, 2011-10-24 at 10:11 +0300, Pekka Enberg wrote:
>> Looking at the code, it's obviously broken for HashMap.put() with a null key.
>
> Urgh yes.
>
>> @@ -345,7 +345,10 @@ public class HashMap extends AbstractMap
>>
>> while (e != null)
>> {
>> - if ((key.hashCode() == e.key.hashCode()) && equals(key, e.key))
>> + int hash1 = key == null ? 0 : key.hashCode();
>> + int hash2 = e.key == null ? 0 : e.key.hashCode();
>> +
>> + if ((hash1 == hash2) && equals(key, e.key))
>> {
>> e.access(); // Must call this for bookkeeping in LinkedHashMap.
>> V r = e.value;
>
> Are you sure that is right?
> What about a key which isn't null but has a hashCode() of zero?
Does that matter? The hashCode check will be a false positive but
equals() should catch it, right?
Re: [cp-patches] [RFC/PATCH] Fix NPE in HashMap.put()
On Mon, 2011-10-24 at 10:24 +0200, Mark Wielaard wrote:
> On Mon, 2011-10-24 at 10:11 +0300, Pekka Enberg wrote:
> > Looking at the code, it's obviously broken for HashMap.put() with a null
> > key.
>
> Urgh yes.
>
> > @@ -345,7 +345,10 @@ public class HashMap extends AbstractMap
> >
> > while (e != null)
> >{
> > -if ((key.hashCode() == e.key.hashCode()) && equals(key, e.key))
> > +int hash1 = key == null ? 0 : key.hashCode();
> > +int hash2 = e.key == null ? 0 : e.key.hashCode();
> > +
> > +if ((hash1 == hash2) && equals(key, e.key))
> >{
> > e.access(); // Must call this for bookkeeping in LinkedHashMap.
> > V r = e.value;
>
> Are you sure that is right?
> What about a key which isn't null but has a hashCode() of zero?
Doh. That wouldn't be a problem, because that would still trigger the
equals(key, e.key) to be called... OK, I think this is the right fix.
Thanks,
Mark
Re: [cp-patches] [RFC/PATCH] Fix NPE in HashMap.put()
On Mon, 2011-10-24 at 10:11 +0300, Pekka Enberg wrote:
> Looking at the code, it's obviously broken for HashMap.put() with a null key.
Urgh yes.
> @@ -345,7 +345,10 @@ public class HashMap extends AbstractMap
>
> while (e != null)
>{
> -if ((key.hashCode() == e.key.hashCode()) && equals(key, e.key))
> +int hash1 = key == null ? 0 : key.hashCode();
> +int hash2 = e.key == null ? 0 : e.key.hashCode();
> +
> +if ((hash1 == hash2) && equals(key, e.key))
>{
> e.access(); // Must call this for bookkeeping in LinkedHashMap.
> V r = e.value;
Are you sure that is right?
What about a key which isn't null but has a hashCode() of zero?
Thanks,
Mark
[cp-patches] [RFC/PATCH] Fix NPE in HashMap.put()
Stefan Ring reports that commit f154af6 ("Fix HashMap.put() to check for
hashCode equality before equals()") breaks running the CACAO test suite for me.
It looks like this:
LD_LIBRARY_PATH=../../../src/cacao/.libs ../../../src/cacao/cacao
-Xbootclasspath:../../../src/classes/classes:/home/sr/classpathcvs/share/classpath/glibj.zip
-classpath /usr/share/java/junit4.jar:. org.junit.runner.JUnitCore All
JUnit version 4.5
.E
Time: 0.003
There was 1 failure:
1) initializationError(All)
java.lang.NullPointerException
at java.util.HashMap.put(HashMap.java:348)
at java.util.HashSet.add(HashSet.java:151)
at org.junit.runners.model.RunnerBuilder.addParent(RunnerBuilder.java:64)
at org.junit.runners.model.RunnerBuilder.runners(RunnerBuilder.java:81)
at org.junit.runners.Suite.(Suite.java:88)
at org.junit.runners.Suite.(Suite.java:54)
at java.lang.reflect.VMConstructor.construct(Native Method)
at java.lang.reflect.Constructor.newInstance(Constructor.java:318)
at
org.junit.internal.builders.AnnotatedBuilder.buildRunner(AnnotatedBuilder.java:35)
at
org.junit.internal.builders.AnnotatedBuilder.runnerForClass(AnnotatedBuilder.java:24)
at
org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:57)
at
org.junit.internal.builders.AllDefaultPossibilitiesBuilder.runnerForClass(AllDefaultPossibilitiesBuilder.java:29)
at
org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:57)
at org.junit.runners.model.RunnerBuilder.runners(RunnerBuilder.java:93)
at org.junit.runners.model.RunnerBuilder.runners(RunnerBuilder.java:84)
at org.junit.runners.Suite.(Suite.java:66)
at org.junit.runner.Request.classes(Request.java:68)
at org.junit.runner.JUnitCore.run(JUnitCore.java:107)
at org.junit.runner.JUnitCore.runMain(JUnitCore.java:88)
at org.junit.runner.JUnitCore.runMainAndExit(JUnitCore.java:54)
at org.junit.runner.JUnitCore.main(JUnitCore.java:46)
FAILURES!!!
Tests run: 1, Failures: 1
Looking at the code, it's obviously broken for HashMap.put() with a null key.
Signed-off-by: Pekka Enberg
---
java/util/HashMap.java |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/java/util/HashMap.java b/java/util/HashMap.java
index 9d64fec..4e92731 100644
--- a/java/util/HashMap.java
+++ b/java/util/HashMap.java
@@ -345,7 +345,10 @@ public class HashMap extends AbstractMap
while (e != null)
{
-if ((key.hashCode() == e.key.hashCode()) && equals(key, e.key))
+int hash1 = key == null ? 0 : key.hashCode();
+int hash2 = e.key == null ? 0 : e.key.hashCode();
+
+if ((hash1 == hash2) && equals(key, e.key))
{
e.access(); // Must call this for bookkeeping in LinkedHashMap.
V r = e.value;
--
1.7.6.4
