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

[Volker Simonis]

On Thu, 27 Apr 2023 21:24:04 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
>
> 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