RFR: JDK-8068427 Hashtable deserialization reconstitutes table with wrong capacity
Peter Levart
peter.levart at gmail.com
Wed Jan 21 20:43:07 UTC 2015
Thanks Martin, Mike, Chris and Daniel,
This has been pushed.
Regards, Peter
On 01/21/2015 05:44 PM, Martin Buchholz wrote:
> Looks good to me!
>
> On Wed, Jan 21, 2015 at 5:04 AM, Peter Levart <peter.levart at gmail.com
> <mailto:peter.levart at gmail.com>> wrote:
>
> Hi Martin and others,
>
> I have also modified the test per Martin's suggestion to factor
> out the serialClone() method. Here's the latest webrev:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Hashtable.8068427/webrev.03/
> <http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/Hashtable.8068427/webrev.03/>
>
> Is this ok now?
>
> Regards, Peter
>
>
> On 01/09/2015 11:16 AM, Peter Levart wrote:
>
> 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/
> <http://cr.openjdk.java.net/%7Eplevart/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-request at openjdk.java.net
> <mailto:core-libs-dev-request at 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.fuchs at oracle.com
> <mailto:daniel.fuchs at oracle.com>>
> To: Peter Levart <peter.levart at gmail.com
> <mailto:peter.levart at gmail.com>>, core-libs-dev
> <core-libs-dev at openjdk.java.net
> <mailto:core-libs-dev at openjdk.java.net>>
> Subject: Re: RFR: JDK-8068427 Hashtable
> deserialization reconstitutes
> table with wrong capacity
> Message-ID: <54AAA547.8070804 at oracle.com
> <mailto:54AAA547.8070804 at 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/
> <http://cr.openjdk.java.net/%7Eplevart/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
>
>
>
>
>
>
More information about the core-libs-dev
mailing list