RFR: 8232846: ProcessHandle.Info command with non-English shows question marks
Toshio 5 Nakamura
TOSHIONA at jp.ibm.com
Fri Mar 27 14:52:18 UTC 2020
Hi Roger,
Thank you so much for the review and being sponsor this fix.
Best regards,
Toshio
> From: Roger Riggs <Roger.Riggs at oracle.com>
>
> 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 <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