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

[Volker Simonis]

On Thu, 20 Apr 2023 10:57:30 GMT, Aleksey Shipilev <shade 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.
>
> test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 43:
> 
>> 41: public class NegativeArraySizeTest {
>> 42: 
>> 43:     private static byte[] _buildPayload() throws IOException {
> 
> `_buildPayload` -> `buildPayload`.

Done.

> test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 44:
> 
>> 42: 
>> 43:     private static byte[] _buildPayload() throws IOException {
>> 44:         String[] simpleArray = new String[1];
> 
> Inline this variable?

Done.

> test/jdk/java/io/ObjectInputStream/NegativeArraySizeTest.java line 55:
> 
>> 53:         // Find the right location to modify, looking for the first instance of TC_ENDBLOCKDATA
>> 54:         int firstPos = 0;
>> 55:         for (int i=0; i<serializedData.length-1; i++) {
> 
> Suggestion:
> 
>         for (int i = 0; i < serializedData.length - 1; i++) {

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1175171971
PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1175173884
PR Review Comment: https://git.openjdk.org/jdk/pull/13540#discussion_r1175174823