RFR: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character [v2]

[Ichiroh Takiguchi]

On Wed, 27 Apr 2022 17:20:11 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> Ichiroh Takiguchi has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8285517: System.getenv() returns unexpected value if environment variable has non ASCII character
>
> src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 68:
> 
>> 66:     private static final Map<String,String> theUnmodifiableEnvironment;
>> 67:     static final int MIN_NAME_LENGTH = 0;
>> 68:     static final Charset cs = Charset.forName(StaticProperty.nativeEncoding(),
> 
> cs should have an all-CAPS name to make it clear its a static value throughout the implementation.
> And `private` too.

Change to "private static final"

> test/jdk/java/lang/System/i18nEnvArg.java line 41:
> 
>> 39: 
>> 40: public class i18nEnvArg {
>> 41:     final static String text = "\u6F22\u5B57";
> 
> Can this be renamed to indicate it is the string under test?  like KANJI_TEXT or EUC_JP_TEXT or similar.

Use EUC_JP_TEXT

> test/jdk/java/lang/System/i18nEnvArg.java line 49:
> 
>> 47:     final static int maxSize = 4096;
>> 48: 
>> 49:     public static void main(String[] args) throws Exception {
> 
> There are 3 main()'s in this test; it would be useful to add a comment indicating the purpose of each.

Add comments

> test/jdk/java/lang/System/i18nEnvArg.java line 60:
> 
>> 58:         Process p = pb.start();
>> 59:         InputStream is = p.getInputStream();
>> 60:         byte[] ba = new byte[maxSize];
> 
> You can use `InputStream.readAllBytes()` here to return a byte buffer.
> 
> Its not clear why all the bytes are read?  I assume its only for a possible error from the child.

Use InputStream.readAllBytes()

> test/jdk/java/lang/System/i18nEnvArg.java line 128:
> 
>> 126:             Method enviorn_mid = cls.getDeclaredMethod("environ");
>> 127:             enviorn_mid.setAccessible(true);
>> 128:             byte[][] environ = (byte[][]) enviorn_mid.invoke(null,
> 
> typo: "enviorn_mid" -> "environ_mid".

Fix typo

> test/jdk/java/lang/System/i18nEnvArg.java line 138:
> 
>> 136:                 bb.put(environ[i+1]);
>> 137:                 byte[] envb = Arrays.copyOf(ba, bb.position());
>> 138:                 if (Arrays.equals(kanji, envb)) return;
> 
> It seems odd to exit the loop on the first match.
> 
> I think AIX always has environment variables not set by the caller.

On this testing, use 2 environment variables:
* LANG=ja_JP.eucjp
* \u6F22\u5B57=\u6F22\u5B57

If the other environment variable is defined, it's printed.

> test/jdk/java/lang/System/i18nEnvArg.java line 146:
> 
>> 144:                         sb.append((char)b);
>> 145:                     } else {
>> 146:                         sb.append(String.format("\\x%02X", (int)b & 0xFF));
> 
> The methods of java.util.HexFormat may be useful here.
> It seems that the "5C" would fit into this "else" clause and not need a separate statement.

Use HexFormat class

-------------

PR: https://git.openjdk.java.net/jdk/pull/8378