RFR: 8232846: ProcessHandle.Info command with non-English shows question marks

[Thomas Stüfe]

Looks good to me in its current form, thanks.

On Fri, Mar 27, 2020 at 8:02 AM Toshio 5 Nakamura <TOSHIONA at jp.ibm.com>
wrote:

> Hi Thomas,
>
> Yes, I tested it locally with small buffer size, 50 for example. But, it's
> hard to create a jtreg test.
>
> I confirmed ERROR_INSUFFICIENT_BUFFER returned from GetLastError(), if the
> provided buffer was not enough.
> Furthermore, "lpdwSize" parameter didn't set a new value in that case.
> I hope the manual has more specific description...
>
> Best regards,
> Toshio Nakamura
>
>
> ----- Original message -----
>
> Hi Toshio,
>
> did you test the malloc path? Since
> https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-queryfullprocessimagenamew#return-value does
> not mention ERROR_INSUFFICIENT_BUFFER as return code, would like to see
> this working. It does however mention that it returns the number of
> returned characters, so if the return code does not work out we may use
> that.
>
> If you tested the truncation path and it works I am fine with the change
> as it is.
>
> Cheers, Thomas
>
>
> On Fri, Mar 27, 2020 at 6:08 AM Toshio 5 Nakamura <TOSHIONA at jp.ibm.com>
> 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
> > >>>>>>>
> > >>>
> > >
> >
>
>
>
>