And post hoc looks fine to me as well.
Mike > On Jan 21, 2015, at 12:43 PM, Peter Levart <peter.lev...@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.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 >
