search

quick-perf / quickperf

QuickPerf is a testing library for Java to quickly evaluate and improve some performance-related properties
https://github.com/quick-perf/doc/wiki/QuickPerf
Apache License 2.0
489 stars 65 forks source link

Potential NPE #160

Closed FranckDemeyer closed 3 years ago

FranckDemeyer commented 3 years ago

Describe the bug Hi, À potential NPE appears in JvmOptionConverter#jvmOptionExists(JvmOptions jvmOptions) A call to jvmOptions.values() is done and a check to JvmOption != nullis done afterward which may lead to a NPE

See directly : quickperf/jvm/jvm-annotations/src/main/java/org/quickperf/jvm/JvmOptionConverter.java

Versions

jeanbisutti commented 3 years ago

Hi @FranckDemeyer,

Thank you very much for this report!

I don't think that there is a bug but the code could be simplified.

In the following method, I don't think that jvmOptions parameter could be null:

    private boolean jvmOptionsExist(JvmOptions jvmOptions) {
        String jvmOptionsAsAString = jvmOptions.value();
        return jvmOptions != null && !jvmOptionsAsAString.isEmpty();
    }

The code may be simplified like this:

    private boolean jvmOptionsExist(JvmOptions jvmOptions) {
        String jvmOptionsAsAString = jvmOptions.value();
        return !jvmOptionsAsAString.isEmpty();
    }

Let me explain.

Each JVM annotation has the ability to add JVM options.

In the JvmAnnotationsConfigs class, you have the following piece of code allowing to configure the JvmOptions annotation: 

   static final AnnotationConfig JVM_OPTIONS = new AnnotationConfig.Builder()
            .testHasToBeLaunchedInASpecificJvm(JvmOptionsAnnotToJvmOptionConverter.INSTANCE)
            .build(JvmOptions.class);

The second line says that the test has to be launched on a JVM only used by the test method. There is a JvmOptionsAnnotToJvmOptionConverter given in parameter to the method of the second line to specify how the JVM options have to be extracted from the JvmOptions annotation. So the JvmOptions annotation has no reason to be null when the converter is used.

Would you be interested in providing a pull request to simplify the code?

FranckDemeyer commented 3 years ago

Hi @jeanbisutti, Because I only read the code directly from github without pulling the code on my IDE, I wasn't sure if JvmOptions could be null or not. It would be a pleasure to contribute. I'll send a PR as soon as I can.

jeanbisutti commented 3 years ago

It would be a pleasure to contribute. I'll send a PR as soon as I can.

Great @FranckDemeyer! When you will have the possibility to do it.

FranckDemeyer commented 3 years ago

@jeanbisutti : https://github.com/quick-perf/quickperf/pull/162

jeanbisutti commented 3 years ago

Fixed with 18665ddd30b459929deabfb73778d0f64608c5d9. Thank you @FranckDemeyer!