On 01/05/2015 05:52 PM, Mike Duigou wrote:
Well spotted Peter. The change looks good though I wonder if it should
be:
int length = (int)((elements + elements / 20) / loadFactor) + 3;
FYI, regarding Daniel's suggestion: When similar invariant checks were
added to the HashMap deserialization method we found code which relied
upon the illegal values. In some cases the serialized HashMaps were
created by alternative serialization implementations which included
illegal, but until the checks were added, "harmless" values.
The invariant checks should still be added though. It might even be
worth adding checks that the other deserialized values are in valid
ranges.
Mike
Hi Mike and others,
Yes, your suggested length computation is more in "spirit" with the
comment above that states: "Compute new length with a bit of room 5% to
grow...", since it takes loadFactor into account for that additional 5%
too. I changed it to your suggested expression.
Here's new webrev:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Hashtable.8068427/webrev.02/
In addition to valid loadFactor, # of elements is checked to be
non-negative. The original length is just "repaired" if it appears to be
less than the enforced auto-growth invariant of Hashtable.
I also expanded the test to exercise more combinations of # of elements
and loadFactor. Here's what gets printed with current Hashtable
implementation:
ser. deser.
size load lentgh length valid range ok?
------- ----- ------- ------- ----------------- ------
10 0.15 127 4 67... 134 NOT-OK
10 0.50 31 8 21... 42 NOT-OK
10 0.75 15 10 14... 28 NOT-OK
10 1.00 15 13 11... 22 OK
10 2.50 7 7 5... 10 OK
50 0.15 511 12 334... 668 NOT-OK
50 0.50 127 30 101... 202 NOT-OK
50 0.75 127 42 67... 134 NOT-OK
50 1.00 63 55 51... 102 OK
50 2.50 31 31 21... 42 OK
500 0.15 4095 103 3334... 6668 NOT-OK
500 0.50 1023 278 1001... 2002 NOT-OK
500 0.75 1023 403 667... 1334 NOT-OK
500 1.00 511 511 501... 1002 OK
500 2.50 255 255 201... 402 OK
5000 0.15 65535 1003 33334... 66668 NOT-OK
5000 0.50 16383 2753 10001... 20002 NOT-OK
5000 0.75 8191 4003 6667... 13334 NOT-OK
5000 1.00 8191 5253 5001... 10002 OK
5000 2.50 2047 2047 2001... 4002 OK
With patched Hashtable, the test passes:
ser. deser.
size load lentgh length valid range ok?
------- ----- ------- ------- ----------------- ------
10 0.15 127 69 67... 134 OK
10 0.50 31 23 21... 42 OK
10 0.75 15 15 14... 28 OK
10 1.00 15 13 11... 22 OK
10 2.50 7 7 5... 10 OK
50 0.15 511 349 334... 668 OK
50 0.50 127 107 101... 202 OK
50 0.75 127 71 67... 134 OK
50 1.00 63 55 51... 102 OK
50 2.50 31 23 21... 42 OK
500 0.15 4095 3501 3334... 6668 OK
500 0.50 1023 1023 1001... 2002 OK
500 0.75 1023 703 667... 1334 OK
500 1.00 511 511 501... 1002 OK
500 2.50 255 213 201... 402 OK
5000 0.15 65535 35003 33334... 66668 OK
5000 0.50 16383 10503 10001... 20002 OK
5000 0.75 8191 7003 6667... 13334 OK
5000 1.00 8191 5253 5001... 10002 OK
5000 2.50 2047 2047 2001... 4002 OK
Regards, Peter
On 2015-01-05 07:48, core-libs-dev-requ...@openjdk.java.net wrote:
Today's Topics:
2. Re: RFR: JDK-8068427 Hashtable deserialization reconstitutes
table with wrong capacity (Daniel Fuchs)
Message: 2
Date: Mon, 05 Jan 2015 15:52:55 +0100
From: Daniel Fuchs <daniel.fu...@oracle.com>
To: Peter Levart <peter.lev...@gmail.com>, core-libs-dev
<core-libs-dev@openjdk.java.net>
Subject: Re: RFR: JDK-8068427 Hashtable deserialization reconstitutes
table with wrong capacity
Message-ID: <54aaa547.8070...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
On 04/01/15 18:58, Peter Levart wrote:
Hi,
While investigating the following issue:
https://bugs.openjdk.java.net/browse/JDK-8029891
I noticed there's a bug in deserialization code of java.util.Hashtable
(from day one probably):
https://bugs.openjdk.java.net/browse/JDK-8068427
The fix is a trivial one-character replacement: '*' -> '/', but I also
corrected some untruthful comments in the neighbourhood (which might
have been true from day one, but are not any more):
http://cr.openjdk.java.net/~plevart/jdk9-dev/Hashtable.8068427/webrev.01/
Hi Peter,
I wonder whether there should be a guard against loadFactor being
zero/negative/NaN after line 1173, like in the constructor e.g. as
in lines
188 if (loadFactor <= 0 || Float.isNaN(loadFactor))
189 throw new IllegalArgumentException("Illegal Load:
"+loadFactor);
if only to avoid division by zero.
best regards,
-- daniel
Regards, Peter
