Review request: JDK-8016579 (process) IOException thrown by ProcessBuilder.start() method is incorrectly encoded (v.3)

[Alexey Utkin]

On 7/17/2013 10:29 PM, Dan Xu wrote:
> I don't know the difference between (*env)->NewStringUTF(env, buf) and 
> JNU_NewStringPlatform(env, buf). Does the JNU_NewStringPlatform() 
> function currently fail to process non-western locale? According to 
> the code, JNU_NewStringPlatform() is also trying to use the right 
> encoding for the give string.
JNU_NewStringPlatform() is using the String(byte[] bytes) constructor. 
Javadoc says: "Constructs a new |String| by decoding the specified array 
of bytes using the platform's default charset ... The behavior of this 
constructor when the given bytes are not valid in the default charset is 
unspecified". In the most of cases the code page is "ISO-8859-1" by 
hysterical reasons (Win9x/WinMe) of user does not change the Java system 
property.

Currently in Java for Windows (in libraries and JVM, but not in AWT - I 
& Artem fixed it there) we got stupid decoding pipe:
win32_UTF16(aka Windows Unicode)(JNI - 16bit WCHAR) -> ISO-8859-1(JVM - 
8bit char) -> UTF16(Java - 16bit jchar).
It can be compared with TV stream HDTV->MPG4->HDTV.

Instead, (*env)->NewStringUTF(env, buf) call goes by pipe
win32_UTF16(aka Windows Unicode)(JNI) -> UTF8 (JVM multibyte 8bit!)-> 
UTF16(Java) without information loss and has no dependance from user's 
fantasy about system locale.

Regards,
-uta

> Other changes look good to me. Thanks for fixing getLastErrorString 
> function in io_util_md.c file.
>
> -Dan
>
> On 07/17/2013 08:28 AM, Alexey Utkin wrote:
>> Thanks,
>>
>> Here is new version with Dan's correction:
>> http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8016579/webrev.02/
>>
>> Regards,
>> -uta
>>
>>
>> On 7/16/2013 11:07 PM, Dan Xu wrote:
>>> I agree. Is jdk_util.c or jni_util.c or any file under 
>>> native/common/ a good place?
>>>
>>> The fix looks good to me.
>>>
>>> One small comment is to get the return value of swprintf() in 
>>> win32Error(). If the return value is -1, an error situation needs to 
>>> be handled even though it seems very rare. If it is not -1, then get 
>>> the actual length of utf16_javaMessage. And pass the length of 
>>> utf16_javaMessage to WideCharToMultiByte() instead of MESSAGE_LENGTH 
>>> to make the code more efficient. Thanks!
>> Accepted.
>>>
>>> -Dan
>>>
>>>
>>>
>>>
>>> On 07/16/2013 10:49 AM, Martin Buchholz wrote:
>>>> Looks OK to me.  As ever, we continue to not have good places 
>>>> within the JDK itself for C-level infrastructure - win32Error is 
>>>> not specific to the process api, and should really be pulled into 
>>>> some common directory - but I don't know of any such.
>>>>
>>>>
>>>> On Tue, Jul 16, 2013 at 9:04 AM, Alexey Utkin 
>>>> <alexey.utkin at oracle.com <mailto:alexey.utkin at oracle.com>> wrote:
>>>>
>>>>     Here is new version of fix:
>>>>     http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8016579/webrev.01/
>>>>     <http://cr.openjdk.java.net/%7Euta/openjdk-webrevs/JDK-8016579/webrev.01/>
>>>>
>>>>     On 7/15/2013 9:08 PM, Martin Buchholz wrote:
>>>>>     Superficial review:
>>>>>
>>>>>     Looks good mostly.
>>>>>
>>>>>     Historically, switching windows code to use "W" APIs has been
>>>>>     a big TODO, but was waiting for Win98 de-support.
>>>>     In java.lang we have passed the half-way: the error reporting
>>>>     sub-system is in the ASCII world.
>>>>>
>>>>>     Please spell correctly:
>>>>>     MESAGE_LENGTH
>>>>     Thanks, I missed it. Fixed.
>>>>>
>>>>>     If errno and GetLastError are two separate error notification
>>>>>     systems, how do you know which one corresponded to the last
>>>>>     failure?  E.g. if the last failure only set errno, won't the
>>>>>     error message be via GetLastError(), which is likely to be stale?
>>>>     As Dan mentioned, the os_lasterror was a copy of JVM 
>>>>     os::lasterror call.
>>>>     The error message procedure is used for
>>>>        CreatePipe
>>>>        CreateProcessW
>>>>        GetExitCodeProcess
>>>>        WaitForMultipleObjects
>>>>     fail report. You are right, all the calls return the problem by
>>>>     GetLastMessage call.
>>>>     The function is changed (reduced).
>>>>
>>>>     Here is new version of fix:
>>>>     http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8016579/webrev.01/
>>>>     <http://cr.openjdk.java.net/%7Euta/openjdk-webrevs/JDK-8016579/webrev.01/>
>>>>
>>>>     Regards,
>>>>     -uta
>>>>
>>>>>
>>>>>
>>>>>
>>>>>     On Mon, Jul 15, 2013 at 2:41 AM, Alexey Utkin
>>>>>     <alexey.utkin at oracle.com <mailto:alexey.utkin at oracle.com>> wrote:
>>>>>
>>>>>         Bug description:
>>>>>         https://jbs.oracle.com/bugs/browse/JDK-8016579
>>>>>         http://bugs.sun.com/view_bug.do?bug_id=8016579
>>>>>
>>>>>         Here is the suggested fix:
>>>>>         http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8016579/webrev.00/
>>>>>         <http://cr.openjdk.java.net/%7Euta/openjdk-webrevs/JDK-8016579/webrev.00/>
>>>>>
>>>>>         Summary:
>>>>>         We have THREE locales in action:
>>>>>         1. Thread default locale - dictates UNICODE-to-8bit conversion
>>>>>         2. OS locale that defines the message localization
>>>>>         3. The file name locale
>>>>>
>>>>>         Each locale could be an extended locale, that means that
>>>>>         text cannot be mapped to 8bit sequence without multibyte
>>>>>         encoding. VM is ready for that, if text is UTF-8.
>>>>>         The suggested fix does the work right from the beginning.
>>>>>
>>>>>         Unicode version of JVM call:
>>>>>          hotspot/src/os/windows/vm/os_windows.cpp:
>>>>>                  size_t os::lasterror(char* buf, size_t len)
>>>>>         was used as prototype for Unicode error message getter. It
>>>>>         has to be fixed accordingly as well as
>>>>>          jdk/src/windows/native/java/io/io_util_md.c
>>>>>                  size_t getLastErrorString(char *buf, size_t len)
>>>>>
>>>>>         The bug contains the attachment
>>>>>         https://jbs.oracle.com/bugs/secure/attachment/14581/JDK-8016579.txt
>>>>>         that summarize the fix result in comparison with original
>>>>>         implementation.
>>>>>
>>>>>         Regards,
>>>>>         -uta
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>