On Thu, 27 Apr 2023 21:24:04 GMT, Roger Riggs <[email protected]> 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
>
> Thanks for the investigation.
>
> On the question of the exception thrown IllegalObjectException vs
> StreamCorruptedExeception, I'd lean toward StreamCorruptedException,
> including for the current PR; as that is more indicative of the issue raised.
>
> As for addressing the existing uses of checkArray that would throw NAE, I
> would rather see a single fix in checkArray than adding code in multiple
> other places. A fix in checkArray would cover future uses as well as current
> uses. The existing code that is checking len < 0 before calling checkArray
> can continue to do so to maintain compatibility on the exception thrown.
> Though a change would be unlikely to break user code it is better to avoid
> that. (It might cause changes existing tests).
>
> Handling it in a separate PR may be reasonable but it too will require a CSR
> (change in behavior from throwning NAE to SCE) and the cause and behavior
> change are generally the same as the current PR. If handled in a single
> PR/CSR and release note will have a more coherent and singular explanation.
Hi @RogerRiggs,
I have now updated both, `checkArray()` and `readArray()` to throw a
`StreamCorruptedException("Array length is negative")` in the case of a
negative array size. I've also changed the signature of
`ObjectInputStream::checkArray()` and `JavaObjectInputStreamAccess::checkArray`
to throw an `ObjectStreamException` instead of an `InvalidClassException`
because `ObjectStreamException` is the super class of both
`InvalidClassException` and `StreamCorruptedException`. Finally I've updated
the test with an extra case of a `PriorityQueue` with a negative element size
in order to test `checkArray()`.
OK now? Once we read consensus here I'll update the CSR accordingly.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13540#issuecomment-1531464609
