On Wed, 26 Apr 2023 12:47:47 GMT, Roger Riggs <rri...@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
