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

[Thomas Stuefe]

On Tue, 29 Aug 2023 18:40:23 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> 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 cal...

Nice, thanks. I double-checked `CHECK_EXCEPTION_RETURN_VALUE` to make sure it really checks also for null and not only for `env->ExceptionOccurred`.

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

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