RFR: 8232846: ProcessHandle.Info command with non-English shows question marks
Roger Riggs
Roger.Riggs at oracle.com
Fri Mar 27 14:24:14 UTC 2020
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:
> http://cr.openjdk.java.net/~tnakamura/8232846/webrev.02/
>
> Thanks,
> Toshio
>
>> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
>>
>> 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 <suenaga at oss.nttdata.com> 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
>>> >>>>>>>>>
More information about the core-libs-dev
mailing list