load classes without initializing them in Converter.CLASS

[farkhalit]

Converter.CLASS was Class::forName, the single-argument overload, which initializes the class it resolves. Any option declared with type Class reaches it, so -c some.Klass runs that class's static initializer during parsing, before the application has looked at the returned Class object at all. Resolving a name should not execute code.

The three-argument overload with initialize set to false gives the same defining class loader and the same ClassNotFoundException contract, and defers initialization to first real use. Converter.OBJECT is unaffected because getConstructor().newInstance() initializes anyway.

Reachable through PatternOptionBuilder pattern +, TypeHandler.createClass, and CommandLine.getParsedOptionValue; the new test fails on master and passes with the change.

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
• Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

[garydgregory]

@farkhalit
Please explain why is PR is needed.

[farkhalit]

Because resolving a class name currently executes code from that class.

Converter.CLASS is Class::forName, the one-arg overload, which initializes the class. So any Class-typed option runs the static initializer of whatever name shows up in argv, during parsing, before the caller has even looked at the returned Class.

Repro against master:

public class Evil {
    static { System.out.println("STATIC INITIALIZER RAN"); }
    public Evil() {}
}

public class Repro {
    public static void main(String[] a) throws Exception {
        Options o = PatternOptionBuilder.parsePattern("c+");
        CommandLine cl = new DefaultParser().parse(o, new String[] {"-c", "Evil"});
        System.out.println("before getParsedOptionValue");
        Class<?> k = cl.getParsedOptionValue("c");
        System.out.println("got class object: " + k.getName());
    }
}

javac -cp target/classes -d . Evil.java Repro.java && java -cp .:target/classes Repro prints before getParsedOptionValue, then STATIC INITIALIZER RAN, then got class object: Evil. Same path via TypeHandler.createClass.

With the three-arg overload and initialize false, the initializer line is gone and the Class object is still returned; initialization happens on first real use instead. Same defining class loader, same ClassNotFoundException contract. Converter.OBJECT is unchanged since getConstructor().newInstance() initializes anyway.

Happy to drop it if you consider running argv-named static initializers at parse time acceptable, but it looked wrong to me.

[garydgregory]

Hello @farkhalit
I am aware of the low-level details but I fail to understand how this protects an application using Commons CLI. Please explain.

[farkhalit]

Fair question. It is not a security boundary on its own: if someone can already control argv and put a class on your classpath, they have other options. What it changes is ordering, and that is what an application can actually use.

The sane way to consume a Class-typed option is parse first, validate, then use:

Class<?> c = cl.getParsedOptionValue("c");
if (!Plugin.class.isAssignableFrom(c)) {
    throw new ParseException("not a plugin: " + c.getName());
}
Plugin p = (Plugin) c.getConstructor().newInstance();

On master that check is dead code from a protection standpoint. The static initializer of whatever name appeared in argv has already run inside getParsedOptionValue, before the application ever sees the Class object. So an application doing everything right still cannot gate anything: the code ran during parsing. With initialize false, the app receives a resolved Class with nothing from it executed, and initialization happens only if and when the app decides to instantiate or touch a static. The validation point goes back to the caller, where it belongs.

Secondary, more mundane benefit: eager initialization means a class whose static init is heavy or throws surfaces as ExceptionInInitializerError / NoClassDefFoundError out of the parser instead of something the application can handle, and you pay that cost even for classes the app then rejects. Reflection APIs generally split resolve from initialize for exactly this reason; Class.forName(String) is just the convenience overload that folds them together.

Converter.OBJECT keeps the current behavior since newInstance() initializes anyway, so nothing regresses there.

If you would rather keep the current semantics, happy to close it.

[raboof]

The sane way to consume a Class-typed option is parse first, validate, then use:

(...)

The validation point goes back to the caller, where it belongs.

You keep stating that as fact, but you don't really back that up with motivation.

The advantages of doing the initialization early (the current behavior) are:

• predictability: when there's something wrong it fails on startup, rather than later down the line where the code path could be dependent on other options, circumstances and timing
• problem detection: when you introduce a problem in a static initializer, you immediately notice that problem, instead of only noticing it when you test the specific code path that touches that option
• consistency: aborting before doing any work is less likely to leave the system in an inconsistent state compared to starting processing and only crashing when the problematic class is touched.

The advantages of doing the initialization lazily (this PR) are:

• performance: potentially faster startup, especially in invocations where static initializers are heavy and the relevant classes/options are not actually used
• resilience: in situations where one class is causing trouble, you can still use the tool as long as you avoid the option associated with that class.
• error handling: as you mention, an opportunity for the application to add error handling for options that are known to be potentially problematic

I think I like the these advantages, but it's not obvious that it's better, right?

[garydgregory]

@farkhalit
If the isAssignableFrom() call is your use case, then please add a test that shows that this example indeed doesn't initialize the class. I'm still uncertain of the benefits but seeing this use case in action would be better.

[garydgregory]

@farkhalit ping 🔔