Looks good to me! On Wed, Jan 21, 2015 at 5:04 AM, Peter Levart <peter.lev...@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-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 >>>>> >>>>> >>>> >>> >> >
