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

[Thomas Stüfe]

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://urldefense.proofpoint.com/v2/url?u=https-3A__docs.microsoft.com_en-2Dus_windows_win32_api_winbase_nf-2Dwinbase-2Dqueryfullprocessimagenamew&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ&s=oc_KTGmJUjsfnLf3tjWr9DO1dwWp1TqIZgd2EiHVW14&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=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ&s=pn_uS0DojBk6-R6R5QwtYFJvJpaabhia-eiOtIrJO9Q&e=
>
> > >>>>
> > >>>>>>> Webrev:
> > >>>>
>
> https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.00&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ&s=NylYK0Q3nFFZRVIaeUna6iaClaJm1CWJ2Zkwy-ysvv4&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
> > >>>>>>>
> > >>>
> > >
> >
>