Hi Roger,
Thank you so much for the review and being sponsor this fix. Best regards, Toshio > From: Roger Riggs <[email protected]> > > Hi Toshio, > > Looks good, > > I can sponsor it. > > Thanks, Roger > > On 3/27/20 6:46 AM, Toshio 5 Nakamura wrote: > > Thank you for review, Suenaga-san, Thomas. > > > > (My mail in html format was blocked by the list. Sorry for inconvenience.) > > > > The latest webrev is 02: > > https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.02_&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=NbJbA8eszoW-rk4brJaNlSeZiHqZPeE8XFp2akykxQU&s=o0kV8G3aphifHI_0bBpRcnkyOBvB9i63nXBUiS1-mjY&e= > > > > Thanks, > > Toshio > > > >> From: Yasumasa Suenaga <[email protected]> > >> > >> 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: > > https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.02&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=PIzjJwY4DhVqnxGp-4WusnNS4MlBy9PEGoCiCE9Hnaw&e= > > > >>> 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: > > https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.01&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=sSZ6mylujPpWlHliOmnz9ukEJoaJrOnLJWuTptntVj8&e= > >>> > > >>> > 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 be limit > >>> > [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 probablytoo 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://urldefense.proofpoint.com/v2/url?u=https-3A__docs.microsoft.com_en-2Dus_windows_win32_api_winbase_nf-2Dwinbase-2Dqueryfullprocessimagenamew&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=V6oP8SJvlxIc2iOxIOAcY6_mznTaxta2tyCV2iVzD3I&e= > >>> >>>>>>>> > >>> >>>>>>>> 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://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8232846&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=8iQnOHT0GkoAZsYE39mDmx8036dUvvDbcj4tjCaPyps&e= > >>> >>>>>> > >>> >>>>>>>>> Webrev: > > https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.00&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=0avs_hvCJdUYCP8WPu8Ttlo4sLwbwuFs2wWV0Szc9gg&e= > > > >>> >>>>>>>>> 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 > >>> >>>>>>>>> >
