RFR: JDK-8068427 Hashtable deserialization reconstitutes table with wrong capacity
Martin Buchholz
martinrb at google.com
Wed Jan 21 16:44:50 UTC 2015
Looks good to me!
On Wed, Jan 21, 2015 at 5:04 AM, Peter Levart <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/
>
> 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/
>>
>> 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 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>
>>>> To: Peter Levart <peter.levart at gmail.com>, core-libs-dev
>>>> <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>
>>>> 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
>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list