RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v2]

[Roger Riggs]

On Wed, 16 Mar 2022 17:45:16 GMT, Tyler Steele <duke at openjdk.java.net> wrote:

>> As described in the linked issue, NullClassBytesTest fails due an OutOfMemoryError produced on AIX when the test calls defineClass with a byte array of size of 0. The native implementation of defineClass then calls  malloc with a size of 0. On AIX malloc(0) returns NULL, while on other platforms it return a valid address. When NULL is produced by malloc for this reason, ClassLoader.c incorrectly interprets this as a failure due to a lack of memory.
>> 
>> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when `errno == ENOMEM` and to produce a ClassFormatError with the message "ClassLoader internal allocation failure" in all other cases (in which malloc returns NULL).~~ [edit: The above no longer describes the PR's proposed fix. See discussion below]
>> 
>> In addition, I performed some minor tidy-up work in ClassLoader.c by changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` to `if (some_ptr == NULL)`. This was done to improve the clarity of the code in ClassLoader.c, but didn't feel worthy of opening a separate issue.
>> 
>> ### Alternatives
>> 
>> It would be possible to address this failure by modifying the test to accept the OutOfMemoryError on AIX. I thought it was a better solution to modify ClassLoader.c to produce an OutOfMemoryError only when the system is actually out of memory.
>> 
>> ### Testing
>> 
>> This change has been tested on AIX and Linux/x86.
>
> Tyler Steele has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - Fixes type warning.
>  - Removes unneeded ClassFormatError from ClassLoader.c
>  - Revert "Extract memory error logic to helper procedure"
>    
>    This reverts commit b631eb0ccd5f3748c2010c864f8ccef0c1da9c42.
>  - Avoid calling malloc with size zero.

Changes requested by rriggs (Reviewer).

src/java.base/share/native/libjava/ClassLoader.c line 106:

> 104:     // NULL or a unique non-NULL pointer. To unify libc behavior across our platforms
> 105:     // we chose the latter. (see 8283225)
> 106:     body = (jbyte *)malloc(length < 1 ? 1 : length);

This code conflates a length == in the comment with length < 1 in the code.
If the issue is with length == 0, make that be the test.

src/java.base/share/native/libjava/ClassLoader.c line 250:

> 248:     // NULL or a unique non-NULL pointer. To unify libc behavior across our platforms
> 249:     // we chose the latter. (see 8283225)
> 250:     body = (jbyte *)malloc(length < 1 ? 1 : length);

The comment talks about a length of zero, but the code does something else with length < 1.
I'd rather not see negative sizes conflated with zero.

test/hotspot/jtreg/runtime/DefineClass/NullClassBytesTest.java line 26:

> 24: /*
> 25:  * @test
> 26:  * @bug 8262913 8283225

All of the changes can be removed from NullClassBytesTest.java.  (copyright and bug #)

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

PR: https://git.openjdk.java.net/jdk/pull/7829