JDK 8 RFR 8016252: More defensive HashSet.readObject

[Brian Burkhalter]

On Oct 8, 2013, at 1:51 AM, Chris Hegarty wrote:

> This looks much better.
> 
> loadfactor and size validation look good, and in line with the original suggestion in the bug.
> 
> For the initial capacity what was originally suggested was to use a similar technique to that of HashMap.readObject ( to ensure that the table never needs to be rehashed during load, and ends up being at least 25% full). Snippet from the bug:
> 
>   int capacity = (int)Math.min(size * Math.min(1 / loadFactor, 4.0f),
>                                HashMap.MAXIMUM_CAPACITY);
> 
> I'm curious why you have not taken that suggestion? What you have looks ok, but it does reply on reasonable capacity and loadfactor values.

I saw that suggestion. I was trying insofar as possible to use the field values as serialized. After reading your comments above however I think that the suggestion is better. I have updated the webrev accordingly

http://cr.openjdk.java.net/~bpb/8016252/webrev.3/

Note that this is a new URL.

> I have not looked in any detail at the test, but the file should start with the copyright/header, and not the import statements.


Fixed - thanks.

On Oct 8, 2013, at 2:14 AM, Alan Bateman wrote:

> This looks better, the checks on loadFactor and size look okay.
> 
> I don't know all the history on this bug but it looks like the goal was partly to avoid resizing when deserializing. This means the capacity could match HashMap. To be consistent with the size check then we could throw an exception if the capacity is negative (interesting case for tests and malware only).

Negative capacity check added in the updated webrev.3.

> I skimmed over the test but it doesn't appear to exercise anything new. If you want to exercise the checks then it would require deserializing from a byte stream that results in bad loadFactor, size and capacity values. It might not be worth it of course.


No, it does not exercise anything new. I actually did try inserting random garbage into the written-out byte stream, but without knowing where to do so to affect the fields of interest it is rather useless and causes totally unpredictable results. I don't know that this test really needs to be included with the patch.

Thanks,

Brian