[jdk11u-dev] RFR: 8315135: Memory leak in the native implementation of Pack200.Unpacker.unpack() [v2]

[Volker Simonis]

On Tue, 29 Aug 2023 14:44:24 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Volker Simonis has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed indentation in test
>
> src/java.base/share/classes/com/sun/java/util/jar/pack/UnpackerImpl.java line 123:
> 
>> 121:                        // Free up native memory and JNI handles to prevent leaks
>> 122:                        ((NativeUnpack) _nunp).finish();
>> 123:                     }
> 
> What happens if `NativeUnpack` already called `finish()`? In case an error happened in the window after it calling `finish()` and it returning? Would that be harmless?

The normal control flow if no error happens is that `NativeUnpack::run()` calls `NativeUnpack::finish()` before returning to its caller `UnpackerImpl::unpack()`. You are right that with these changes `NativeUnpack::finish()` will now be called two times in the case of successful unpacking. That's not a problem, because `NativeUnpack::finish()` is implemented as follows:

JNIEXPORT jlong JNICALL
Java_com_sun_java_util_jar_pack_NativeUnpack_finish(JNIEnv *env, jobject pObj) {
  unpacker* uPtr = get_unpacker(env, pObj, false);
  CHECK_EXCEPTION_RETURN_VALUE(uPtr, 0);
  size_t consumed = uPtr->input_consumed();
  free_unpacker(env, pObj, uPtr);
  return consumed;
}

and `get_unpacker(env, pObj, false)` will always return a valid pointer to a `unpacker` object (or throw an exception). That's controlled by the third argument, which basically instructs `get_unpacker()` to create a new `unpacker` object if the current one is NULL. `free_unpacker(env, pObj, uPtr)` in a subsequent line will than release all the `unpacker`'s native memory and set the unpacker field (i.e. `NativeUnpack::unpackerPtr`) to NULL.

So while calling `NativeUnpack::finish()` repeatedly is correct, it is unnecessarily expensive because we may create a new native `unpacker` object in `Java_com_sun_java_util_jar_pack_NativeUnpack_finish()` just to delete it right afterwards. I've therefore changed `Java_com_sun_java_util_jar_pack_NativeUnpack_finish()` as follows:

JNIEXPORT jlong JNICALL
Java_com_sun_java_util_jar_pack_NativeUnpack_finish(JNIEnv *env, jobject pObj) {
  // There's no need to create a new unpacker here if we don't already have one
  // just to immediatly free it afterwards.
  unpacker* uPtr = get_unpacker(env, pObj, /* noCreate= */ true);
  CHECK_EXCEPTION_RETURN_VALUE(uPtr, 0);
  size_t consumed = uPtr->input_consumed();
  // free_unpacker() will set the unpacker field on 'pObj' to null
  free_unpacker(env, pObj, uPtr);
  return consumed;
}

Now, if we call `get_unpacker()` with the third argument set to `true`, `get_unpacker()` will not create a new `unpacker` object any more if the `NativeUnpack::unpackerPtr` is already NULL. The subsequent `CHECK_EXCEPTION_RETURN_VALUE` will check for a NULL return value and simple return from `Java_com_sun_java_util_jar_pack_NativeUnpack_finish()`.

PS: the third, boolean argument `noCreate` of `get_unpacker()` is a default argument with the default value being `false`. All other call sites of `get_unpacker()` use this default, only the call from `Java_com_sun_java_util_jar_pack_NativeUnpack_finish()` explicitly adds the third argument as `false` which is already the default anyway. I strongly suspect that should have been `true` from the very beginning, because it makes no sense to create a new `unpacker` object just to delete it again a few lines later.

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

PR Review Comment: https://git.openjdk.org/jdk11u-dev/pull/2100#discussion_r1309215728