RFR: 8232846: ProcessHandle.Info command with non-English shows question marks
Yasumasa Suenaga
suenaga at oss.nttdata.com
Fri Mar 27 09:52:04 UTC 2020
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 <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 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
> >>>>>>>>>
> >>>>>
> >>>
> >>
>
>
More information about the core-libs-dev
mailing list