RFR: JDK-8068427 Hashtable deserialization reconstitutes table with wrong capacity
Mike Duigou
mike at duigou.org
Thu Jan 22 06:37:33 UTC 2015
And post hoc looks fine to me as well.
Mike
> On Jan 21, 2015, at 12:43 PM, Peter Levart <peter.levart at gmail.com> wrote:
>
> 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> 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