Ok, your change looks good.
Thanks, Yasumasa On 2020/03/27 18:10, Toshio 5 Nakamura wrote:
Hi Suenaga-san, Thank you for the comment. I updated the limit to 32768. webrev.02: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.02 Well, I believe the new logic works as same as the current one. I added NULL initialization to commandObj, and the value will be changed only if QueryFullProcessImageNameW() was success. Then, CHECK_NULL(commandObj) works as "return" if commandObj is NULL. src/java.base/share/native/libjava/jni_util.h > #define CHECK_NULL(x) \ > do { \ > if ((x) == NULL) { \ > return; \ > } \ > } while (0) \ I hope this solves your concern. Best regards, Toshio Nakamura ----- Original message ----- Nakamura-san, I think location of CHECK_NULL and SetObjectField() are incorrect. Currently they are called when QueryFullProcessImageName() succeed only. In addition, you need to allocate 32768 chars (32767 + 1 ('\0')) via malloc. I understand "32767" is max length, not includes null-terminator. Thanks, Yasumasa On 2020/03/27 14:06, Toshio 5 Nakamura wrote: > > Hi Roger, Suenaga-san, > > Thank you for your comments and discussion for the issue. > I've updated webrev. Could you review it again? > tier1 tests passed. > > webrev.01: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.01 > > Best regards, > Toshio Nakamura > > Yasumasa Suenaga <[email protected]> wrote on 2020/03/27 09:10:15: > >> On 2020/03/27 9:01, Roger Riggs wrote: >>> Hi, >>> >>> Please don't throw an exception. >>> It would abort being able to provide any information. >>> And who would expect or handle such an exception? >>> >>> Degrading the info or omitting it provides better service overall. >>> There is no point to throwing an exception. >> >> hProcess in QueryFullProcessImageNameW needs >> PROCESS_QUERY_INFORMATION or PROCESS_QUERY_LIMITED_INFORMATION >> access right, so I thought it is reasonable to throw runtime >> exception if the call failed. >> >> Anyway, I agree with you if throwing exception is not comfortable in >> this case for Java API. >> >> >> Thanks, >> >> Yasumasa >> >> >>> Regards, Roger >>> >>> >>> On 3/26/20 7:55 PM, Yasumasa Suenaga wrote: >>>> On 2020/03/27 0:35, Roger Riggs wrote: >>>>> Hi, >>>>> >>>>> Reading further down the reference to the section: >>>>> "Enable Long Paths in Windows 10, Version 1607, and Later" >>>>> might suggest the code be more resilient to long paths. >>>>> >>>>> The stack allocated buffer (1024) is fine, but I'd suggest >> adding code to retry in the case >>>>> of INSUFFICIENT BUFFER with a malloc'd buffer to make it more >> robust since the enabling >>>>> of long paths is environment and application specific. >>>> >>>> In addition, it's better to throw an exception if the call failed >> due to other error code. >>>> >>>> Thanks, >>>> >>>> Yasumasa >>>> >>>> >>>>> Thanks, Roger >>>>> >>>>> >>>>> >>>>> On 3/26/20 10:40 AM, Toshio 5 Nakamura wrote: >>>>>> Hi Thomas, Suenaga-san, Roger, >>>>>> >>>>>> Thank you for reviewing and comments. >>>>>> >>>>>> I checked behaviors by a small program and debugger. >>>>>> If QueryFullProcessImageNameW failed by not enough buffer, >>>>>> the API didn't change the buffer. >>>>>> It just set ERROR_INSUFFICIENT_BUFFER(0x7a) to LastError. >>>>>> >>>>>> The buffer size becomes 1024 characters (2048 bytes) by this patch. >>>>>> Should it use bigger size with malloc? 32,767 characters can belimit > [2]. >>>>>> I feel 1024 is reasonable value for command's location. >>>>>> >>>>>> [2] >>>>>> > https://urldefense.com/v3/__https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file*maximum-path-length-limitation__;Iw!!GqivPVa7Brio!PgzCcUQOjCNpcVeetU_drS4DFFVLaj0ceJBvipX7iDc_RlPtcEkdH8AzelGtzqXS > $ >>>>>> Best regards, >>>>>> Toshio Nakamura >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> If the call fails, the command field in the info will not be set (and >>>>>>> therefore null). >>>>>>> >>>>>>> I agree that a length of 512 (characters) is probably too small. >>>>>>> >>>>>>> Roger >>>>>>> >>>>>>> >>>>>>> On 3/26/20 6:37 AM, Yasumasa Suenaga wrote: >>>>>>>> Hi Nakamura-san, >>>>>>>> >>>>>>>> Your change almost looks good, but I have one question. >>>>>>>> Length of exeName (1024) is enough? >>>>>>>> >>>>>>>> According to Microsoft Doc [1], exeName might not be null-terminated >>>>>>>> if it failed. >>>>>>>> I concern buffer overrun might occur when QueryFullProcessImageNameW >>>>>>>> failed. >>>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Yasumasa >>>>>>>> >>>>>>>> >>>>>>>> [1] >>>>>> > https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-queryfullprocessimagenamew > >>>>>> >>>>>>>> >>>>>>>> On 2020/03/26 17:58, Toshio 5 Nakamura wrote: >>>>>>>>> Hi All, >>>>>>>>> >>>>>>>>> Could you review this change? Additionally, I'd like to ask a > sponsor >>>>>>>>> for >>>>>>>>> it, since I'm not a committer. >>>>>>>>> >>>>>>>>> Bug: >>>>>> > https://bugs.openjdk.java.net/browse/JDK-8232846 > >>>>>> >>>>>>>>> Webrev: >>>>>> > http://cr.openjdk.java.net/~tnakamura/8232846/webrev.00 >>>>>>>>> Under Windows environments, ProcessHandle.Info.command shows question marks >>>>>>>>> if the command has non-English characters. I'd like to change the >>>>>>>>> underlying API to Unicode version. >>>>>>>>> Tier1 tests passed. >>>>>>>>> >>>>>>>>> Best regards, >>>>>>>>> Toshio Nakamura >>>>>>>>> >>>>> >>> >>
