Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8285517: System.getenv() returns unexpected value if environment variable has non ASCII character #8378

Closed
wants to merge 9 commits into from
+240 −47
Closed

8285517: System.getenv() returns unexpected value if environment variable has non ASCII character #8378

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
89ee195
8285517: System.getenv() returns unexpected value if environment vari…
takiguc Apr 24, 2022
050dd66
8285517: System.getenv() returns unexpected value if environment vari…
takiguc May 1, 2022
9db7987
8285517: System.getenv() returns unexpected value if environment vari…
takiguc May 3, 2022
84e6d63
8285517: System.getenv() returns unexpected value if environment vari…
takiguc May 4, 2022
5bd8492
8285517: System.getenv() returns unexpected value if environment vari…
takiguc May 6, 2022
994a7fd
8285517: System.getenv() returns unexpected value if environment vari…
takiguc May 7, 2022
6dbaa75
Merge branch 'master' into 8285517
takiguc May 9, 2022
1201801
8285517: System.getenv() returns unexpected value if environment vari…
takiguc May 13, 2022
b940a0d
8285517: System.getenv() returns unexpected value if environment vari…
takiguc May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion src/java.base/share/classes/jdk/internal/util/StaticProperty.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -26,6 +26,7 @@
package jdk.internal.util;

import java.util.Properties;
import java.nio.charset.Charset;

/**
* System Property access for internal use only.
@@ -52,6 +53,8 @@ public final class StaticProperty {
private static final String NATIVE_ENCODING;
private static final String FILE_ENCODING;
private static final String JAVA_PROPERTIES_DATE;
private static final String SUN_JNU_ENCODING;
private static final Charset jnuCharset;

private StaticProperty() {}

@@ -69,6 +72,8 @@ private StaticProperty() {}
NATIVE_ENCODING = getProperty(props, "native.encoding");
FILE_ENCODING = getProperty(props, "file.encoding");
JAVA_PROPERTIES_DATE = getProperty(props, "java.properties.date", null);
SUN_JNU_ENCODING = getProperty(props, "sun.jnu.encoding");
jnuCharset = Charset.forName(SUN_JNU_ENCODING, Charset.defaultCharset());
}
Comment on lines +76 to 77
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure it is OK to initialize Charset here, as sun_jnu_encoding is initialized in System.initPhase1() and pulling Charset there may cause some init order change. I'd only cache the encoding string here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the initialization of sun.jnu.encoding in System.java:2142'ish.
This happens before StaticProperty is initialized; at line 2147.
If the sun.jnu.encoding is not supported (this is before Providers are enabled)
then it is forced to UTF-8.
So it is the case that the encoding is supported by that point.
Note that Charset.isSupported(...) does the full lookup in its implementation.

If it is not supported, the system still comes up using UTF-8 and a warning is printed at line 2282.

Comparing the class initialization order may be a useful thing to cross check.


private static String getProperty(Properties props, String key) {
@@ -241,4 +246,29 @@ public static String fileEncoding() {
public static String javaPropertiesDate() {
return JAVA_PROPERTIES_DATE;
}

/**
* Return the {@code sun.jnu.encoding} system property.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be eliminated by changing the@return block tag below to {@return the {@code sun.jnu.encoding} ...}.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @naotoj .
I appreciate your comment.
I'd like to confirm one thing.
I applied following change

    /**
     * {@return the {@code sun.jnu.encoding} system property}
     *
     * <strong>{@link SecurityManager#checkPropertyAccess} is NOT checked
     * in this method. The caller of this method should take care to ensure
     * that the returned property is not made accessible to untrusted code.</strong>
     */

By javadoc command with -html5 option, above part was converted to

Returns the sun.jnu.encoding system property. SecurityManager.checkPropertyAccess(java.lang.String) 
is NOT checked in this method. The caller of this method should take care to ensure that the returned 
property is not made accessible to untrusted code.

The 1st word changed from Return to Returns.
Is it OK ?
And it seems @return is translated to Japanese on Japanese environment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 1st word changed from Return to Returns.
Is it OK ?

That's more of plain English to me. Maybe some natives can correct it if needed.

And it seems @return is translated to Japanese on Japanese environment.

Yes. I believe that's what is supposed to work.

*
* <strong>{@link SecurityManager#checkPropertyAccess} is NOT checked
* in this method. The caller of this method should take care to ensure
* that the returned property is not made accessible to untrusted code.</strong>
*
* @return the {@code sun.jnu.encoding} system property
*/
public static String jnuEncoding() {
return SUN_JNU_ENCODING;
}

/**
* Return charset for {@code sun.jnu.encoding} system property.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. charset can be capitalized and changed to {@code}.

*
* <strong>If {@code sun.jnu.encoding} system property has invalid
* encoding name, {@link Charset#defaultCharset()} is returned.</strong>
*
* @return charset for {@code sun.jnu.encoding} system property
*/
public static Charset jnuCharset() {
return jnuCharset;
}
}
13 changes: 2 additions & 11 deletions src/java.base/unix/classes/java/lang/ProcessEnvironment.java
View file
Original file line number Diff line number Diff line change
@@ -56,25 +56,16 @@

import java.io.*;
import java.nio.charset.Charset;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.*;
import jdk.internal.util.StaticProperty;


final class ProcessEnvironment
{
private static final HashMap<Variable,Value> theEnvironment;
private static final Map<String,String> theUnmodifiableEnvironment;
static final int MIN_NAME_LENGTH = 0;
private static final Charset jnuCharset;
static {
@SuppressWarnings("removal")
String jnuEncoding = AccessController.doPrivileged((PrivilegedAction<String>) ()
-> System.getProperty("sun.jnu.encoding"));
jnuCharset = jnuEncoding != null
? Charset.forName(jnuEncoding, Charset.defaultCharset())
: Charset.defaultCharset();
}
private static final Charset jnuCharset = StaticProperty.jnuCharset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the Charset is cached in StaticProperty, I don't think its worthwhile to make a local copy of the same ref.
Just refer to StaticProperty.jnuCharset() where it is needed. (Here and in ProcessImpl)


static {
// We cache the C environment. This means that subsequent calls
10 changes: 1 addition & 9 deletions src/java.base/unix/classes/java/lang/ProcessImpl.java
View file
Original file line number Diff line number Diff line change
@@ -142,15 +142,7 @@ static Platform get() {

private static final Platform platform = Platform.get();
private static final LaunchMechanism launchMechanism = platform.launchMechanism();
private static final Charset jnuCharset;
static {
@SuppressWarnings("removal")
String jnuEncoding = AccessController.doPrivileged((PrivilegedAction<String>) ()
-> System.getProperty("sun.jnu.encoding"));
jnuCharset = jnuEncoding != null
? Charset.forName(jnuEncoding, Charset.defaultCharset())
: Charset.defaultCharset();
}
private static final Charset jnuCharset = StaticProperty.jnuCharset();
private static final byte[] helperpath = toCString(StaticProperty.javaHome() + "/lib/jspawnhelper");

private static byte[] toCString(String s) {
73 changes: 30 additions & 43 deletions test/jdk/java/lang/System/i18nEnvArg.java
View file
Original file line number Diff line number Diff line change
@@ -32,14 +32,11 @@
* @run main i18nEnvArg
*/

import java.io.File;
import java.io.ByteArrayOutputStream;
import java.io.InputStream;
import java.io.PrintStream;
import java.lang.reflect.Method;
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
import java.util.Arrays;
import java.util.ArrayList;
import java.util.HexFormat;
import java.util.List;
import java.util.Map;
@@ -54,20 +51,18 @@ public class i18nEnvArg {
*/
public static void main(String[] args) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 main()'s in this test; it would be useful to add a comment indicating the purpose of each.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comments

ProcessBuilder pb;
List<String> cmds = new ArrayList<>();
cmds.addAll(List.of(
"--add-modules=" + System.getProperty("test.modules"),
"-classpath",
System.getProperty("test.class.path"),
"-Dtest.class.path=" + System.getProperty("test.class.path"),
"-Dtest.modules=" + System.getProperty("test.modules")));
if (args.length == 0) {
var cmds = List.of(
"--add-modules=" + System.getProperty("test.modules"),
"-classpath",
System.getProperty("test.class.path"),
"-Dtest.jdk=" + System.getProperty("test.jdk"),
"-Dtest.class.path=" + System.getProperty("test.class.path"),
"-Dtest.modules=" + System.getProperty("test.modules"),
"i18nEnvArg",
"Start");
pb = ProcessTools.createTestJvm(cmds);
Map<String, String> environ = pb.environment();
environ.clear();
environ.put("LANG", "ja_JP.eucjp");
cmds.addAll(
List.of("-Dtest.jdk=" + System.getProperty("test.jdk"),
"i18nEnvArg",
"Start"));
} else {
String jnuEncoding = System.getProperty("sun.jnu.encoding");
Charset dcs = jnuEncoding != null
@@ -77,23 +72,22 @@ public static void main(String[] args) throws Exception {
if (!dcs.equals(cs)) {
return;
}
var cmds = List.of(
"--add-modules=" + System.getProperty("test.modules"),
"--add-opens=java.base/java.lang=ALL-UNNAMED",
"-classpath",
System.getProperty("test.class.path"),
"i18nEnvArg$Verify",
EUC_JP_TEXT);
pb = ProcessTools.createTestJvm(cmds);
Map<String, String> environ = pb.environment();
environ.clear();
environ.put("LANG", "ja_JP.eucjp");
cmds.addAll(
List.of("--add-opens=java.base/java.lang=ALL-UNNAMED",
"i18nEnvArg$Verify",
EUC_JP_TEXT));
}
pb = ProcessTools.createTestJvm(cmds);
Map<String, String> environ = pb.environment();
environ.clear();
environ.put("LANG", "ja_JP.eucjp");
if (args.length != 0) {
environ.put(EUC_JP_TEXT, EUC_JP_TEXT);
}
ProcessTools.executeProcess(pb)
.outputTo(System.out)
.errorTo(System.err)
.shouldHaveExitValue(0);
.shouldNotContain("ERROR:");
}

public static class Verify {
@@ -106,21 +100,19 @@ public static void main(String[] args) throws Exception {
byte[] euc = (EUC_JP_TEXT + "=" + EUC_JP_TEXT).getBytes(cs);
byte[] eucjp = "LANG=ja_JP.eucjp".getBytes(cs);
String s = System.getenv(EUC_JP_TEXT);
ByteArrayOutputStream baos = new ByteArrayOutputStream();
PrintStream ps = new PrintStream(baos);
if (!EUC_JP_TEXT.equals(s)) {
ps.println("getenv() returns unexpected data");
System.err.println("ERROR: getenv() returns unexpected data");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the unexpected data s to the output string.

}
if (!EUC_JP_TEXT.equals(args[0])) {
ps.print("Unexpected argument was received: ");
System.err.print("ERROR: Unexpected argument was received: ");
for(char ch : EUC_JP_TEXT.toCharArray()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the expected value, the previous "Unexpected" labeling might be mis-understood.

ps.printf("\\u%04X", (int)ch);
System.err.printf("\\u%04X", (int)ch);
}
ps.print("<->");
System.err.print("<->");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be clearer if labeled as the actual/incorrect value and on a separate line.

for(char ch : args[0].toCharArray()) {
ps.printf("\\u%04X", (int)ch);
System.err.printf("\\u%04X", (int)ch);
}
ps.println();
System.err.println();
}
Class<?> cls = Class.forName("java.lang.ProcessEnvironment");
Method environ_mid = cls.getDeclaredMethod("environ");
@@ -137,15 +129,10 @@ public static void main(String[] args) throws Exception {
byte[] envb = Arrays.copyOf(ba, bb.position());
if (Arrays.equals(eucjp, envb)) continue;
if (!Arrays.equals(euc, envb)) {
ps.println("Unexpected environment variables: " +
System.err.println("ERROR: Unexpected environment variables: " +
hf.formatHex(envb));
}
}
byte[] err = baos.toByteArray();
if (err.length > 0) {
System.err.write(err);
System.exit(1);
}
}
}
}