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.

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

put a null guard around PropertyWriteAction #79

Closed
wants to merge 3 commits into from

Conversation

djeremiah
Copy link
Contributor

classPath may be null when -class option is specified in Main and CLASSPATH environment variable is undefined.

This is the most expedient way to deal with it, but I'm not sure it's the best.
Other options:

  1. remove PropertyWriteAction entirely
    java.class.path is already set by Main if -classpath option is specified but we should probably honor
    CLASSPATH for -class option.
  2. provide a sensible default
    replace line 44 with System.getenv().getOrDefault(). Though I'm not sure what a sensible, non-null default is for java.class.path. Is "" a valid value?

@dmlloyd
Copy link
Contributor

dmlloyd commented Nov 12, 2015

I think "" is a valid value for java.class.path. One way to test that would be to create a test JAR which prints out that property, and then run it as an agent with nothing on the classpath.

@djeremiah
Copy link
Contributor Author

It looks like, at the very least, the Agent.jar is put on the classpath, as well as the jar specified by -jar.

I cooked up an Agent that just prints the property:

public static void premain(String agentargs, Instrumentation inst){
        String path = System.getProperty("java.class.path");
        System.out.println("Path: " + path);
}

Re running the use original use case from the JIRA:

[jboss-eap-6.3]$  java -javaagent:/home/dmurphy/workspaces/jboss-modules/AgentTest/Agent.jar -jar jboss-modules.jar -mp modules/ -dep org.picketbox -class org.picketbox.datasource.security.SecureIdentityLoginModule testString
Path: jboss-modules.jar:/home/dmurphy/workspaces/jboss-modules/AgentTest/Agent.jar
Exception in thread "main" java.lang.NullPointerException
    ...etc...

So I'm not sure about using "" for the default.

@dmlloyd
Copy link
Contributor

dmlloyd commented Nov 13, 2015

Ah, I figured that using an agent would put it on the bootstrap class path instead of the application class path. Also what I meant was that the test program could just run on a raw JVM with no other JARs on the class path.

But perhaps a better test is to run the jar by appending it directly to the boot class path and then running the class by name, again with an empty class path. I may run this test yet tonight if I get a minute.

@dmlloyd
Copy link
Contributor

dmlloyd commented Nov 13, 2015

Figured out a couple interesting facts:

  • The default class path is . - that's one of those facts you learn in your first month of Java and then forget immediately thereafter :-)
  • If you use the bootclasspath hack, you can manually set the class path to empty string

So I think that empty string is the best default.

@dmlloyd
Copy link
Contributor

dmlloyd commented Nov 13, 2015

At least it's better than leaving it unset to default to ????? whatever it might be defaulting to.

@djeremiah
Copy link
Contributor Author

Makes sense to me. PR updated.

}
this.classPath = classPath;
AccessController.doPrivileged(new PropertyWriteAction("java.class.path", classPath));
AccessController.doPrivileged(new PropertyWriteAction("java.class.path", classPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a tab snuck in here?

@dmlloyd
Copy link
Contributor

dmlloyd commented Nov 13, 2015

I squashed it all down to the single one-line diff commit and merged it. Thanks.

@dmlloyd dmlloyd closed this Nov 13, 2015
@djeremiah djeremiah deleted the MODULES-220 branch November 13, 2015 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants