RFR: 8306461: ObjectInputStream::readObject() should handle negative array sizes without throwing NegativeArraySizeExceptions [v3]

[Volker Simonis]

On Wed, 26 Apr 2023 12:47:47 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> Volker Simonis has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8306461
>>  - Simplified exception message and fixed test to check the correct message
>>  - Addresed review comments of @turbanoff, @shipilev and @RogerRiggs
>>  - 8306461: ObjectInputStream::readObject() should handle negative array sizes without throwing NegativeArraySizeExceptions
>
> A bit more investigation is needed.   
> I noticed that ObjectInputStream.checkArray throws NegativeArraySizeException if length is < 0 before calling filterCheck.
> The checkArray method is called via SharedSecrets to check array sizes on some of the collection classes during deserialization.
> Likely, the change should be extended to cover negative lengths in those cases too.

Hi @RogerRiggs ,

I've checked the callers of `checkArray()` and found 13, all of them in the corresponding `readObject()` methods:

| class | NASE | exception |
|:----- |:----:|:--------- |
| `j.u.IdentityHashMap` | + | StreamCorruptedException("Illegal mappings count: " + size) |
| `j.u.HashMap` | + | InvalidObjectException("Illegal mappings count: " + mappings) |
| `j.u.Properties` | + | StreamCorruptedException("Illegal # of Elements: " + elements) |
| `j.u.ArrayList` | + | InvalidObjectException("Invalid size: " + size) |
| `j.u.ImmutableCollections` | + | InvalidObjectException("negative length " + len) |
| `j.u.Hashtable` | + | StreamCorruptedException("Illegal # of Elements: " + elements) |
| `j.u.HashSet` | + | InvalidObjectException("Illegal capacity: " + capacity) |
| `jx.m.o.TabularDataSupport` | (+) | (depends on ArrayList) |
| `j.u.concurrent/PriorityBlockingQueue` | (-) | (depends on PriorityQueue) |
| `j.u.PriorityQueue` | - | |
| `j.u.ArrayDeque` | - | |
| `j.u.concurrent/CopyOnWriteArrayList` | - | |
| `j.u.Collections` | - | |

The occurrences with a "+" int he "NASE" column all already handle the negative array size case **before** calling `ObjectInputStream.checkArray()` and throw the exceptions listed in the "exception" column if the array size is negative.

So instead of changing `ObjectInputStream.checkArray()` maybe it makes sense to fix the remaining four classes (i.e. `PriorityQueue`, `ArrayDeque`, `CopyOnWriteArrayList` and `Collections`) to handle negative array sizes in their corresponding  `readObject()` methods as well? This somehow feels more natural to me, because, other then within the initial issue, this is not a problem of the deserialization protocol, but rather  an issue with custom deserialization code in the the `readObject()` methods. Therefor I also think it would make sense to handle that as a different issue. What do you think?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13540#issuecomment-1525999475