RFR: JDK-8257604: JNI_ArgumentPusherVaArg leaks valist

[Thomas Stuefe]

On Thu, 3 Dec 2020 01:22:29 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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); \
>
> 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. :)

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

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

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