Review request: JDK-8016579 (process) IOException thrown by ProcessBuilder.start() method is incorrectly encoded (v.3)
Dan Xu
dan.xu at oracle.com
Thu Jul 18 17:40:21 UTC 2013
I see. Thanks for your good explanation. The change looks good to me.
-Dan
On 07/18/2013 12:56 AM, Alexey Utkin wrote:
> 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
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list