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

[Roger Riggs]

On Wed, 26 Apr 2023 11:55:23 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> This issue was reported by: Yakov Shafranovich ([yakovsh at amazon.com](mailto:yakovsh at amazon.com))
>> 
>> Currently, `ObjectInputStream::readObject()` doesn't explicitly checks for a negative array length in the deserialization stream. Instead it calls `j.l.r.Array::newInstance(..)` with the negative length which results in a `NegativeArraySizeException`. NegativeArraySizeException is an unchecked exception which is neither declared in the signature of `ObjectInputStream::readObject()` nor mentioned in its API specification. It is therefore not obvious for users of `ObjectInputStream::readObject()` that they may have to handle `NegativeArraySizeException`s. It would therefor be better if a negative array length in the deserialization stream would be automatically wrapped in an `InvalidClassException` which is a checked exception (derived from `IOException` via `ObjectStreamException`) and declared in the signature of `ObjectInputStream::readObject()`.
>> 
>> If we do the negative array length check in `ObjectInputStream::readObject()` before filtering, this will then also fix `ObjectInputFilter.FilterInfo::arrayLength()` which is defined as:
>> 
>> Returns:
>> the non-negative number of array elements when deserializing an array of the class, otherwise -1
>> 
>> but currently returns a negative value if the array length is negative.
>
> 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.

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

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