Skip to content

8276990: Memory leak in invoker.c fillInvokeRequest() during JDI operations #7306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

8276990: Memory leak in invoker.c fillInvokeRequest() during JDI operations #7306

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
52eac5b
8276990: Memory leak in invoker.c fillInvokeRequest() during JDI oper…
Feb 1, 2022
4b62941
Deallocate methodSignature after completed request
Feb 2, 2022
a273574
Deallocate methodSignature in deleteGlobalArgumentRefs()
Feb 2, 2022
812ad32
Preserve methodSignature_init(), we need the cursor later
Feb 2, 2022
b3e7a52
Move freeing of methodSignature after deleteGlobalArgumentRefs()
Feb 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ fillInvokeRequest(JNIEnv *env, InvokeRequest *request,
/*
* Squirrel away the method signature
*/
JDI_ASSERT_MSG(request->methodSignature == NULL, "Request methodSignature not null");
error = methodSignature(method, NULL, &request->methodSignature, NULL);
if (error != JVMTI_ERROR_NONE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be done when the invoke has completed rather than at the start of the next invoke? Otherwise you potentially have one outstanding allocation for every thread, and you still have a leak when the thread exits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be done when the invoke has completed rather than at the start of the next invoke? Otherwise you potentially have one outstanding allocation for every thread, and you still have a leak when the thread exits.

Yes, perhaps. Please help me, where would be a good and reliable place to do that? Towards the end of invoker_doInvoke() ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In invoker_completeInvokeRequest() this appears to be the last reference:

jbyte returnType = methodSignature_returnTag(request->methodSignature);

I would suggest freeing outside of the if (!detached) block and setting it to NULL. You might want to add an assert for NULL where you are currently freeing the pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In invoker_completeInvokeRequest() this appears to be the last reference:

jbyte returnType = methodSignature_returnTag(request->methodSignature);

I would suggest freeing outside of the if (!detached) block and setting it to NULL. You might want to add an assert for NULL where you are currently freeing the pointer.

Alright, that seems sensible. Thank you!
I am not 100% if methodSignature can always be expected to be != NULL there, as I asserted. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to deallocate the methodSignature after deleteGlobalArgumentRefs() because that method accesses it. Or better yet, deallocate it there, because the only point of deleteGlobalArgumentRefs() seems to reset the methodSignature anyway. And that method seems to assume methodSignature != NULL, so we can do the same. WDYT? The change passes tier1 tests (including com/sun/jdi which exercises this code).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you still hitting the assert in fillInvokeRequest()? I'm not sure why it would ever not be NULL there. It's probably worth investigating some more. Otherwise you need to remove the assert, and possibly still have a leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you still hitting the assert in fillInvokeRequest()? I'm not sure why it would ever not be NULL there. It's probably worth investigating some more. Otherwise you need to remove the assert, and possibly still have a leak.

No, the latest version seems good and passes tier1-3 and customer confirmed that it fixes the memory leak.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I think I prefer you to free the memory after calling deleteGlobalArgumentRefs() since deleteGlobalArgumentRefs() is really all about releasing JNI global refs, not freeing other allocated memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've moved it back to after the call to deleteGlobalArgumentRefs().

return error;
Expand Down Expand Up @@ -775,6 +776,10 @@ invoker_completeInvokeRequest(jthread thread)
*/
deleteGlobalArgumentRefs(env, request);

JDI_ASSERT_MSG(request->methodSignature != NULL, "methodSignature is NULL");
jvmtiDeallocate(request->methodSignature);
request->methodSignature = NULL;

/* From now on, do not access the request structure anymore
* for this request id, because once we give up the invokerLock it may
* be immediately reused by a new invoke request.
Expand Down