RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v3]

[Brian Burkhalter]

On Thu, 12 May 2022 07:59:36 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Brian Burkhalter 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:
>> 
>>  - 6478546: Clean up io_util.c
>>  - Merge
>>  - 6478546: Decrease malloc'ed buffer maximum size to 64 kB
>>  - 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available
>
> src/java.base/share/native/libjava/io_util.c line 79:
> 
>> 77:           jint off, jint len, jfieldID fid)
>> 78: {
>> 79:     char stackBuf[STACK_BUF_SIZE];
> 
> This was in the original code already, but doesn't this always allocate 8k on the stack, regardless of whether this buffer will be used (if len > 8k or len == 0)?  Wouldn't it be better to allocate this only in the `else` case?
> 
> Would apply to the write code as well.

This is intentional. We want to avoid having a malloc + free in every call and so avoid it for small buffers.

> src/java.base/share/native/libjava/io_util.c line 81:
> 
>> 79:     char stackBuf[STACK_BUF_SIZE];
>> 80:     char *buf = NULL;
>> 81:     jint buf_size, read_size;;
> 
> minor: double semi colon

FIxed in 10f14bb3faef2b55ab59a85016874d12815f3c87.

> src/java.base/share/native/libjava/io_util.c line 129:
> 
>> 127:             off += n;
>> 128:         } else if (n == -1) {
>> 129:             JNU_ThrowIOExceptionWithLastError(env, "Read error");
> 
> The original code would have `nread` set to `-1` here, and thus exit with `-1`.  The new code would exit with the last value for `nread` which could be anything.

The returned value of `nread` would in this case indicate the number of bytes actually read so far, which is intentional.

> src/java.base/share/native/libjava/io_util.c line 201:
> 
>> 199:         write_size = len < buf_size ? len : buf_size;
>> 200:         (*env)->GetByteArrayRegion(env, bytes, off, write_size, (jbyte*)buf);
>> 201:         if (!(*env)->ExceptionOccurred(env)) {
> 
> Wouldn't this result in an infinite loop if `GetByteArrayRegion` triggered an exception and `len > 0` ?  It would never enter the `if` and `len` is never changed then...

Good catch, you are correct. Fixed in 10f14bb3faef2b55ab59a85016874d12815f3c87.

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

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