RFR: JDK-8068427 Hashtable deserialization reconstitutes table with wrong capacity

[Peter Levart]

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
>>>
>>
>