RFR: JDK-8257604: JNI_ArgumentPusherVaArg leaks valist

[David Holmes]

On Thu, 3 Dec 2020 04:52:48 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

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

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

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

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