Skip to content
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.

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

JDK-8257604: JNI_ArgumentPusherVaArg leaks valist #1565

Conversation

tstuefe
Copy link
Member

@tstuefe tstuefe commented Dec 2, 2020

JNI_ArgumentPusherVaArg copies the given argument list pointer (va_copy) but never releases it. A call to va_end is missing.

AFAICS this coding is old. Interestingly, in jdk8 I find this:

   0:   inline void set_ap(va_list rap) {
   0: #ifdef va_copy
   0:     va_copy(_ap, rap);
   0: #elif defined (__va_copy)
   0:     __va_copy(_ap, rap);
   0: #else
   0:     _ap = rap;
   0: #endif
   0:   }

_ap = rap seems a strangely relaxed way of doing this. But I do not know the history behind that.

I could not find any usage of the original arg pointer, so maybe the va_copy() is not even needed. The jdk8 coding seems to indicate that too. But since I was not 100% sure I kept the va_copy() and added a va_end().

I also made this private (removed the protected) since there are no child classes.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1565/head:pull/1565
$ git checkout pull/1565

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 2, 2020

👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

@tstuefe The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot label Dec 2, 2020
@tstuefe tstuefe force-pushed the JDK-8257604-JNI_ArgumentPusherVaArg-leaks-valist branch from 9858d47 to 4719617 Compare Dec 2, 2020
@tstuefe tstuefe marked this pull request as ready for review Dec 2, 2020
@openjdk openjdk bot added the rfr label Dec 2, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 2, 2020

Webrevs

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Looks good.

I added more history to the bug report.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

@tstuefe This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8257604: JNI_ArgumentPusherVaArg leaks valist

Reviewed-by: dcubed, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • 0b8c780: 8256256: UL should not use heap allocation for output string

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Dec 2, 2020
@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Dec 2, 2020

Looks good.

I added more history to the bug report.

Thanks, Dan. Unfortunately, the issue is not visible to outsiders. But I guess there were platforms where va_copy was not available (like old versions of VS).

coleenp
coleenp approved these changes Dec 2, 2020
Copy link
Contributor

@coleenp coleenp left a comment

Looks good.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Dec 2, 2020

You can read this for historical copying of va_list args:
https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_chapter/libc_34.html#SEC674

But I'm not at all sure that our use of these macros in a RAII helper object is actually valid. From what I've read these macros have to be used in the same function in which the va_list was received.

I'm also not clear why we call va_copy in the first place, as we only need that if the caller also needs to access the va list of arguments. Or perhaps if we need to ensure we actually have something we can pass to another function ... but in that case the va_copy should be in the same function where the va_start is (and the corresponding va_end). (Imagine if args were passed in registers, you'd need to call va_copy to copy them to the stack in the function in which the register still holds the arg.)

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Dec 2, 2020

E.g. I think this macro:

  va_list args; \
  va_start(args, methodID); \
  JavaValue jvalue(Tag); \
  JNI_ArgumentPusherVaArg ap(methodID, args); \
  jni_invoke_nonstatic(env, &jvalue, obj, JNI_VIRTUAL, methodID, &ap, CHECK_0); \
  va_end(args); \

should be:

  va_list args; \
+va_list args_copy; \
  va_start(args, methodID); \
+va_copy(args, args_copy); \
  JavaValue jvalue(Tag); \
! JNI_ArgumentPusherVaArg ap(methodID, args_copy); \
  jni_invoke_nonstatic(env, &jvalue, obj, JNI_VIRTUAL, methodID, &ap, CHECK_0); \
+  va_end(args_copy); \
  va_end(args); \

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Dec 3, 2020

Though technically we still need the current code and proposed fix as we can't just do a direct assignment in the helper.

Correction: technically va_copy and va_end are supposed to occur in the same function, so we're assuming our RAII object gets inlined if we think we're adhering to that.

Perhaps I'm overthinking all this. It may not be "by the book" but it certainly seems to "work" okay so perhaps best not to rock the boat here. :)

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Dec 3, 2020

Hi David,

I think the thoughts are valid, I thought it odd too. The usage of va_args is tightly defined.

https://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdarg.h.html
Posix specifies:
Each invocation of the va_start() and va_copy() macros shall be matched by a corresponding invocation of the va_end() macro in the same function.

I also wondered why we copy in the first place, but as I wrote, I was not sure I missed some caller or some way the original ap got used.

"not rocking the boat" - should I just withdraw this patch? Its a small theoretical leak. But I did a quick test on my Ubuntu box, leaking an va_list, and could not see any memory loss. Of course it may still be leaking on other platforms.

Cheers, Thomas

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Dec 3, 2020

Hi Thomas,

IIUC (and at best that is a partial understanding) the allocation of memory by va_start only happens under specific conditions, and I don't know exactly what they are. I tried going to the source:

https://github.com/gcc-mirror/gcc/blob/master/gcc/builtins.c

but that wasn't exactly illuminating without understanding all the other gcc-isms in use. (You may understand better than I.)

I think your fix is harmless, and technically improves the existing code, even if there are potentially other flaws with how we do this. So if you and the other reviewers want to go ahead that seems fine to me.

Thanks,
David

@tstuefe
Copy link
Member Author

@tstuefe tstuefe commented Dec 3, 2020

Thank you Coleen, David and Dan.

/integrate

@tstuefe tstuefe closed this Dec 3, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 3, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2020

@tstuefe Since your change was applied there have been 13 commits pushed to the master branch:

  • 4169d96: 8257143: Enable JVMCI code installation tests on AArch64
  • a5a034b: 8257617: TestLinkPlatform fails with new Java source version
  • d80ae05: 8166596: TLS support for the EdDSA signature algorithm
  • 3932527: 8257466: Improve enum iteration
  • 02a0a02: 8257563: Remove excessive include of klass.inline.hpp
  • cc1915b: 8253821: Improve ByteBuffer performance with GCM
  • 3da30e9: 8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes
  • 7104400: 8257164: Share LambdaForms for VH linkers/invokers
  • 3e89981: 8257623: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted001/TestDescription.java shouldn't use timeout
  • 93b6ab5: 8256818: SSLSocket that is never bound or connected leaks socket resources
  • ... and 3 more: https://git.openjdk.java.net/jdk/compare/2508bc7c58b631c41f1c2415dd9925d9d309dbe8...master

Your commit was automatically rebased without conflicts.

Pushed as commit ae1eb28.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@tstuefe tstuefe deleted the JDK-8257604-JNI_ArgumentPusherVaArg-leaks-valist branch Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated
4 participants