done all fixed, done report on false positives, wip on true positives

after/entrypoint.sh

@@ -3,4 +3,4 @@
 set -e
 
 cd /tools/home
-/tools/infer/bin/infer -- mvn compile -Drat.skip=true
+/tools/infer/bin/infer -- mvn compile test -Drat.skip=true -Danimal.sniffer.skip=true

after/infer-out/.infer_runstate.json

@@ -1 +1 @@
-{"run_sequence":[{"date":"2023-04-11 09:38:59.468025+02:00","command":"run","infer_version":{"major":1,"minor":1,"patch":0,"commit":"f9b6f2b"}}],"results_dir_format":"db_filename: infer-out/results.db\ndb_schema: \n      CREATE TABLE IF NOT EXISTS procedures\n        ( proc_uid TEXT PRIMARY KEY NOT NULL\n        , proc_name BLOB NOT NULL\n        , attr_kind INTEGER NOT NULL\n        , source_file TEXT NOT NULL\n        , proc_attributes BLOB NOT NULL\n        , cfg BLOB\n        , callees BLOB NOT NULL\n        )\n    ;\n\n      CREATE TABLE IF NOT EXISTS source_files\n        ( source_file TEXT PRIMARY KEY\n        , type_environment BLOB NOT NULL\n        , integer_type_widths BLOB\n        , procedure_names BLOB NOT NULL\n        , freshly_captured INT NOT NULL )\n    ;\n\n      CREATE TABLE IF NOT EXISTS specs\n        ( proc_uid TEXT PRIMARY KEY NOT NULL\n        , proc_name BLOB NOT NULL\n        , analysis_summary BLOB NOT NULL\n        , report_summary BLOB NOT NULL\n        )\n    ;\n\n      CREATE TABLE IF NOT EXISTS model_specs\n        ( proc_uid TEXT PRIMARY KEY NOT NULL\n        , proc_name BLOB NOT NULL\n        , analysis_summary BLOB NOT NULL\n        , report_summary BLOB NOT NULL\n        )\n    ","should_merge_capture":false}
+{"run_sequence":[{"date":"2023-04-12 13:04:24.635363+02:00","command":"run","infer_version":{"major":1,"minor":1,"patch":0,"commit":"f9b6f2b"}}],"results_dir_format":"db_filename: infer-out/results.db\ndb_schema: \n      CREATE TABLE IF NOT EXISTS procedures\n        ( proc_uid TEXT PRIMARY KEY NOT NULL\n        , proc_name BLOB NOT NULL\n        , attr_kind INTEGER NOT NULL\n        , source_file TEXT NOT NULL\n        , proc_attributes BLOB NOT NULL\n        , cfg BLOB\n        , callees BLOB NOT NULL\n        )\n    ;\n\n      CREATE TABLE IF NOT EXISTS source_files\n        ( source_file TEXT PRIMARY KEY\n        , type_environment BLOB NOT NULL\n        , integer_type_widths BLOB\n        , procedure_names BLOB NOT NULL\n        , freshly_captured INT NOT NULL )\n    ;\n\n      CREATE TABLE IF NOT EXISTS specs\n        ( proc_uid TEXT PRIMARY KEY NOT NULL\n        , proc_name BLOB NOT NULL\n        , analysis_summary BLOB NOT NULL\n        , report_summary BLOB NOT NULL\n        )\n    ;\n\n      CREATE TABLE IF NOT EXISTS model_specs\n        ( proc_uid TEXT PRIMARY KEY NOT NULL\n        , proc_name BLOB NOT NULL\n        , analysis_summary BLOB NOT NULL\n        , report_summary BLOB NOT NULL\n        )\n    ","should_merge_capture":false}

after/infer-out/report.txt

@@ -1,22 +1,4 @@
-#0 DONE
+#0
-src/main/java/org/apache/commons/lang3/AnnotationUtils.java:72: error: Null Dereference
-  object returned by `getAllInterfaces(cls)` could be null and is dereferenced at line 72.
-  70.           @Override
-  71.           protected String getShortClassName(final Class<?> cls) {
-  72. >             for (final Class<?> iface : ClassUtils.getAllInterfaces(cls)) {
-  73.                   if (Annotation.class.isAssignableFrom(iface)) {
-  74.                       return "@" + iface.getName();
-
-#1 DONE
-src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java:126: error: Null Dereference
-  object returned by `getAllInterfaces(cls)` could be null and is dereferenced at line 126.
-  124.           // superclass field.
-  125.           Field match = null;
-  126. >         for (final Class<?> class1 : ClassUtils.getAllInterfaces(cls)) {
-  127.               try {
-  128.                   final Field test = class1.getField(fieldName);
-
-#2
 src/main/java/org/apache/commons/lang3/builder/ReflectionToStringBuilder.java:131: error: Null Dereference
   object `null` is dereferenced by call to `toString(...)` at line 131.
   129.        */
@@ -25,7 +7,7 @@ src/main/java/org/apache/commons/lang3/builder/ReflectionToStringBuilder.java:13
   132.       }
   133.   
 
-#3 false positive, LONG_TO_INT_RANGE.fit never returns null (may throw NPE when param is null, never the case though as actual param is boxed primitive long)
+#1
 src/main/java/org/apache/commons/lang3/time/DurationUtils.java:142: error: Null Dereference
   object returned by `org.apache.commons.lang3.time.DurationUtils.LONG_TO_INT_RANGE.fit(valueOf(duration.toMillis()))` could be null and is dereferenced at line 142.
   140.           Objects.requireNonNull(duration, "duration");
@@ -34,7 +16,7 @@ src/main/java/org/apache/commons/lang3/time/DurationUtils.java:142: error: Null
   143.       }
   144.   
 
-#4 false positive, java.lang.String.toCharArray() always returns a non-null char[] object
+#2
 src/main/java/org/apache/commons/lang3/CharSetUtils.java:181: error: Null Dereference
   object `chars` last assigned on line 177 could be null and is dereferenced at line 181.
   179.           final char[] chrs = str.toCharArray();
@@ -43,7 +25,7 @@ src/main/java/org/apache/commons/lang3/CharSetUtils.java:181: error: Null Derefe
   182.                   buffer.append(chr);
   183.               }
 
-#5
+#3
 src/main/java/org/apache/commons/lang3/builder/ToStringBuilder.java:223: error: Null Dereference
   object `null` is dereferenced by call to `ToStringBuilder(...)` at line 223.
   221.        */
@@ -52,71 +34,51 @@ src/main/java/org/apache/commons/lang3/builder/ToStringBuilder.java:223: error:
   224.       }
   225.   
 
-#6 false positive, Validate.notNull(field, ...) throws NPE if field is null thus if the method is call field is non-null
+#4
-src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java:341: error: Null Dereference
+src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java:347: error: Null Dereference
-  object `field` last assigned on line 338 could be null and is dereferenced by call to `readStaticField(...)` at line 341.
+  object `field` last assigned on line 344 could be null and is dereferenced by call to `readStaticField(...)` at line 347.
-  339.           Validate.notNull(field, "Cannot locate field '%s' on %s", fieldName, cls);
+  345.           Validate.notNull(field, "Cannot locate field '%s' on %s", fieldName, cls);
-  340.           // already forced access above, don't repeat it here:
+  346.           // already forced access above, don't repeat it here:
-  341. >         return readStaticField(field, false);
+  347. >         return readStaticField(field, false);
-  342.       }
+  348.       }
-  343.   
+  349.   
 
-#7 false positive, Validate.notNull(field, ...) throws NPE if field is null thus if the method is call field is non-null
+#5
-src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java:385: error: Null Dereference
+src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java:391: error: Null Dereference
-  object `field` last assigned on line 382 could be null and is dereferenced by call to `readStaticField(...)` at line 385.
+  object `field` last assigned on line 388 could be null and is dereferenced by call to `readStaticField(...)` at line 391.
-  383.           Validate.notNull(field, "Cannot locate declared field %s.%s", cls.getName(), fieldName);
+  389.           Validate.notNull(field, "Cannot locate declared field %s.%s", cls.getName(), fieldName);
-  384.           // already forced access above, don't repeat it here:
+  390.           // already forced access above, don't repeat it here:
-  385. >         return readStaticField(field, false);
+  391. >         return readStaticField(field, false);
-  386.       }
+  392.       }
-  387.   
+  393.   
 
-#8 DONE
+#6
-src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java:486: error: Null Dereference
+src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java:605: error: Null Dereference
-  object returned by `primitiveToWrapper(varArgComponentType)` could be null and is dereferenced by call to `newInstance(...)` at line 486.
+  object `field` last assigned on line 602 could be null and is dereferenced by call to `writeStaticField(...)` at line 605.
-  484.           final int varArgLength = args.length - methodParameterTypes.length + 1;
+  603.           Validate.notNull(field, "Cannot locate field %s on %s", fieldName, cls);
-  485.   
+  604.           // already forced access above, don't repeat it here:
-  486. >         Object varArgsArray = Array.newInstance(ClassUtils.primitiveToWrapper(varArgComponentType), varArgLength);
+  605. >         writeStaticField(field, value, false);
-  487.           // Copy the variadic arguments into the varargs array.
+  606.       }
-  488.           System.arraycopy(args, methodParameterTypes.length - 1, varArgsArray, 0, varArgLength);
+  607.   
 
-#9 false positive, Validate.notNull(field, ...) throws NPE if field is null thus if the method is call field is non-null
+#7
-src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java:599: error: Null Dereference
+src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java:650: error: Null Dereference
-  object `field` last assigned on line 596 could be null and is dereferenced by call to `writeStaticField(...)` at line 599.
+  object `field` last assigned on line 647 could be null and is dereferenced by call to `writeField(...)` at line 650.
-  597.           Validate.notNull(field, "Cannot locate field %s on %s", fieldName, cls);
+  648.           Validate.notNull(field, "Cannot locate declared field %s.%s", cls.getName(), fieldName);
-  598.           // already forced access above, don't repeat it here:
+  649.           // already forced access above, don't repeat it here:
-  599. >         writeStaticField(field, value, false);
+  650. >         writeField(field, (Object) null, value, false);
-  600.       }
+  651.       }
-  601.   
+  652.   
 
-#10 false positive, Validate.notNull(field, ...) throws NPE if field is null thus if the method is call field is non-null
+#8
-src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java:644: error: Null Dereference
+src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java:983: error: Null Dereference
-  object `field` last assigned on line 641 could be null and is dereferenced by call to `writeField(...)` at line 644.
+  object `classes` last assigned on line 982 could be null and is dereferenced at line 983.
-  642.           Validate.notNull(field, "Cannot locate declared field %s.%s", cls.getName(), fieldName);
+  981.               final Class<?> mcls = method.getDeclaringClass();
-  643.           // already forced access above, don't repeat it here:
+  982.               final List<Class<?>> classes = getAllSuperclassesAndInterfaces(mcls);
-  644. >         writeField(field, (Object) null, value, false);
+  983. >             for (final Class<?> acls : classes) {
-  645.       }
+  984.                   final Method equivalentMethod = (ignoreAccess ? MethodUtils.getMatchingMethod(acls, method.getName(), method.getParameterTypes())
-  646.   
+  985.                       : MethodUtils.getMatchingAccessibleMethod(acls, method.getName(), method.getParameterTypes()));
 
-#11 false positive, getAllSuperclassesAndInterfaces(mcls) is non-null if mcls is non-null, mcls is non-null as java.lang.reflect.Method.getDeclaringClass() is always non-null
+Found 9 issues
-src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java:987: error: Null Dereference
-  object `classes` last assigned on line 986 could be null and is dereferenced at line 987.
-  985.               final Class<?> mcls = method.getDeclaringClass();
-  986.               final List<Class<?>> classes = getAllSuperclassesAndInterfaces(mcls);
-  987. >             for (final Class<?> acls : classes) {
-  988.                   final Method equivalentMethod = (ignoreAccess ? MethodUtils.getMatchingMethod(acls, method.getName(), method.getParameterTypes())
-  989.                       : MethodUtils.getMatchingAccessibleMethod(acls, method.getName(), method.getParameterTypes()));
-
-#12
-src/main/java/org/apache/commons/lang3/concurrent/MultiBackgroundInitializer.java:160: warning: Thread Safety Violation
-  Read/Write race. Non-private method `MultiBackgroundInitializer.getTaskCount()` reads without synchronization from container `this.childInitializers` via call to `Map.values()`. Potentially races with write in method `MultiBackgroundInitializer.addInitializer(...)`.
- Reporting because another access to the same memory occurs on a background thread, although this access may not.
-  158.           int result = 1;
-  159.   
-  160. >         for (final BackgroundInitializer<?> bi : childInitializers.values()) {
-  161.               result += bi.getTaskCount();
-  162.           }
-
-Found 13 issues
           Issue Type(ISSUED_TYPE_ID): #
-                Null Dereference(NULL_DEREFERENCE): 12
+  Null Dereference(NULL_DEREFERENCE): 9
-  Thread Safety Violation(THREAD_SAFETY_VIOLATION): 1

before/entrypoint.sh

@@ -3,4 +3,4 @@
 set -e
 
 cd /tools/home
-/tools/infer/bin/infer -- mvn compile -Drat.skip=true
+/tools/infer/bin/infer -- mvn compile test -Drat.skip=true -Danimal.sniffer.skip=true

report.tex

@@ -69,7 +69,7 @@ Assignment 1 -- Software Analysis \\\vspace{0.5cm}
 \section{Project selection}
 Given that this assignment draws parallels with the class of Software Design and
 Modelling of last semester, specifically regarding static analyzers, I choose to
-analyze the same project I analyzed in the past with PMD and SonarQube
+analyze the same project I analyzed in the past with PMD and SonarQube using
 Infer\footnote{\url{https://fbinfer.com/}} to make
 for an interesting comparison between static analysis paradigms.
 
@@ -85,8 +85,8 @@ Commons Lang project focuses on classes that would have fitted in the
 
 All the source and test classes are contained within in the package
 \textit{org.apache.commons.lang3} or in a sub-package of that package. For the
-sake of brevity, this prefix is omitted from now on when mentioning packages and
+sake of brevity, this prefix is omitted from now on when mentioning file paths
-classes in the project.
+and classes in the project.
 
 I choose to analyze version 3.12.0 of the library (i.e.\ the code under the
 \textit{git} tag \textit{rel/commons-lang-3.12.0}) because it is the same
@@ -94,9 +94,9 @@ version analyzed during the SDM class.
 
 To verify that the project satisfies the 5000 lines of code requirement, I run
 the \textit{cloc} tool. Results are shown in table \ref{tab:cloc}. Given the
-project has more than 118,000 lines of code, this requirement is satisfied.
+project has more than 86,000 lines of Java code, this requirement is satisfied.
 
-\begin{table}[ht]
+\begin{table}[H]
   \centering
   \begin{tabular}{lrrrr}
     \toprule
@@ -118,8 +118,8 @@ project has more than 118,000 lines of code, this requirement is satisfied.
     Total & 518 & 19,270 & 61,280 & 118,526 \\
     \bottomrule
   \end{tabular}
-  \caption{Output of the \textit{cloc} tool for the Apache Commons Lang project at
+  \caption{Output of the \textit{cloc} tool for the Apache Commons Lang project
-    tag \textit{rel/commons-lang-3.12.0} (before fixes are applied).}
+    at tag \textit{rel/commons-lang-3.12.0} (before fixes are applied).}
   \label{tab:cloc}
 \end{table}
 
@@ -132,9 +132,21 @@ The relevant source code to analyze has been copied to the directory
 \href{https://gitlab.com/usi-si-teaching/msde/2022-2023/software-analysis/maggioni/assignment-2}{\textit{usi-si-teaching/msde/2022-2023/software-analysis/maggioni/assignment-2}}
 \end{center}
 
-on \textit{gitlab.com}. The script \textit{docker-infer.sh} can be ran to
+on \textit{gitlab.com}. The script \textit{docker-infer.sh} can be executed to
 automatically run the Infer tool using default options through the course
-tools docker image \textit{bugcounting/satools:y23}. The analysis outputs
+tools docker image \textit{bugcounting/satools:y23}.
+
+The script selects Java 17 to compile the project as this is required to not
+make the Animal Sniffer Maven build plugin fail with the message ``This feature
+requires ASM7''. 
+
+The script executes Infer in Maven capture mode executing the \textit{compile}
+and \textit{test} targets while disabling the Apache RAT software license
+checker (which fails for this release). Since unit tests are executed, running
+the script before and after the warning guided refactoring ensures the fixes I
+introduce do not introduce regressions.
+
+The analysis outputs
 are located in \textit{before/infer-out/report.txt}.
 
 \section{Results}
@@ -143,47 +155,130 @@ Table \ref{tab:infer} shows the results of the analysis performed by Infer
 providing comments on true and false positives and the actions taken for each
 result.
 
-In total 
+\begin{table}[H]
-
-\begin{table}[]
    \small
-    \begin{tabular}{@{}llp{1.2cm}p{0.8cm}p{6cm}@{}}
+    \begin{tabular}{@{}llp{1.2cm}p{0.65cm}p{6cm}@{}}
 \toprule
-        \textbf{File} & \textbf{Line} & \textbf{Kind} & \textbf{True Pos.} &
+\textbf{File} & \textbf{Line} & \textbf{Kind} & \textbf{True Pos.} &
-        \textbf{Comment} \\ \midrule
+\textbf{Reason why flagged expression is a false positive} \\ \midrule
-AnnotationUtils.java & 72 & Null & Yes & -- \\
+AnnotationUtils.java & 72 & Null & Yes & \multirow{4}{6cm}{--} \\
-reflect/MethodUtils.java & 486 & Null & Yes & -- \\
+reflect/MethodUtils.java & 486 & Null & Yes & \\
-reflect/FieldUtils.java & 126 & Null & Yes & -- \\
+reflect/FieldUtils.java & 126 & Null & Yes & \\
-concurrent/MultiBackgroundInitializer.java & 160 & Thread Safety & Yes & -- \\
+concurrent/MultiBackgroundInitializer.java & 160 & Thread Safety & Yes & \\
 \midrule
-builder/ToStringBuilder.java & 223 & Null & ?? & \multirow{2}{6cm}{??} \\
+builder/ToStringBuilder.java & 223 & Null & No & \multirow{2}{6cm}{Infer flags
+        the value \texttt{null} when used as a nullable method argument} \\
 builder/ReflectionToStringBuilder.java & 131 & Null & No & \\
 \midrule
 time/DurationUtils.java & 142 & Null & No & The method which may return a null
-        value returns a non-null value if its parameter is non-null, and a
+value returns a non-null value if its parameter is non-null, and a
-        non-null parameter is given \\
+non-null parameter is given \\
 \midrule 
-        CharSetUtils.java & 181 & Null & No & According to \textit{java.lang} documentation, the
+CharSetUtils.java & 181 & Null & No & According to \textit{java.lang}
-        method always returns a non-null value \\
+documentation, the method always returns a non-null value \\
 \midrule
 reflect/FieldUtils.java & 341 & Null & No &
- \multirow{4}{6cm}{a utility method is used to guard the dereference reported
+\multirow{4}{6cm}{A utility method is used to guard the dereference reported
-        with an exception throw} \\
+with an exception throw} \\
- reflect/FieldUtils.java & 385 & Null & No & \\
+reflect/FieldUtils.java & 385 & Null & No & \\
- reflect/FieldUtils.java & 599 & Null & No & \\
+reflect/FieldUtils.java & 599 & Null & No & \\
- reflect/FieldUtils.java & 644 & Null & No & \\
+reflect/FieldUtils.java & 644 & Null & No & \\
 \midrule
 reflect/MethodUtils.java & 987 & Null & No & The method which may return a null
-        value returns a non-null value if its parameter is non-null, and a
+value returns a non-null value if its parameter is non-null, and a
-        non-null parameter is always given according to the \textit{java.lang}
+non-null parameter is always given according to the \textit{java.lang}
-        documentation for the inner nested method \\ \bottomrule
+documentation for the inner nested method \\ \bottomrule
 \end{tabular}
     \caption{Results of the Infer static analysis tool execution with default
     options. \textit{True Pos.} denotes whether a result is a true positive,
     while \textit{Kind} denotes with \textit{Null} and \textit{Thread Safety}
-    respectively null dereference issues and thread safety violations.}
+    respectively null dereference warnings and thread safety violations.}
     \label{tab:infer}
 \end{table}
 
+In total Infer reports 13 warnings, 12 of which are null dereference warnings
+and 1 is a thread safety violation. Of all warnings, 4 are true positives and 9
+are false positives, resulting in a false positive ratio of over 69\%. These
+values are summarized in table \ref{tab:num}.
+
+\begin{table}[H]
+    \centering
+\begin{tabular}{@{}ll@{}}
+\toprule
+Total number of warnings:  & 13 \\ \midrule
+Null dereference warnings: & 12 \\
+Thread safety violations:  & 1  \\ \midrule
+True positives:            & 4  \\
+False positives:           & 9  \\ \bottomrule
+\end{tabular}
+    \caption{Quantitative results of the static analysis performed by Infer.}
+    \label{tab:num}
+\end{table}
+
+\subsection{False Positives}
+
+As can be deduced from table \ref{tab:infer}, Infer especially struggles at
+determining Vthe null safety of nested method calls or non-trivial data flow
+paths. This is sometimes caused by insufficient knowledge of the nullability
+contracts of the Java standard library (e.g. \textit{java.lang} package).
+
+In other cases, the flagged expression is guarded by an utility method throwing
+an exception if its value is indeed \texttt{null}. One of such guards is the
+\mintinline{java}{static Validate.notNull(Object, String, Object...)} method,
+which checks if its first argument is null and throws a
+\texttt{NullPointerException} if so.
+
+Additionally, Infer seems to struggle with boxed primitives and not
+understanding that their value is always non-null by construction. As an example
+I provide the warning reported in the file \textit{time/DurationUtils.java}:
+
+\begin{listing}[H]
+\begin{minted}[linenos,firstnumber=139]{java}
+public static int toMillisInt(final Duration duration) {
+    Objects.requireNonNull(duration, "duration");
+    // intValue() does not do a narrowing conversion here
+    return LONG_TO_INT_RANGE.fit(Long.valueOf(duration.toMillis())).intValue();
+}
+\end{minted}
+    \caption{Method \textit{toMillisInt(Duration)} of class
+    \textit{time.DurationUtils} in Apache Commons Lang 3.12.0.}
+\end{listing}
+
+Here Infer reports that the first argument of
+\mintinline{java}{LONG_TO_INT_RANGE.fit(Long)} may be null. However, the return
+value of \mintinline{java}{Long.valueOf(long)} is always non-null since the
+method simply boxes its \mintinline{java}{long} argument.
+
+Finally, some warnings are caused by Infer flagging the use of the \texttt{null}
+keyword as a method argument for methods that would accept a nullable argument
+in that position without causing null dereferences. This warning could point to 
+a potential ambiguity is selecting the right method to call at
+compile time given the argument types (i.e. static dispatching), as
+\texttt{null} is a valid expression of all object types. For example, in the
+given class:
+
+\begin{minted}{java}
+class C {
+  m(String s) {}
+  m(Object o) {}
+}
+\end{minted} 
+
+A call to \mintinline{java}{C.m(null)} would be ambiguous as \texttt{null} is
+both a \texttt{String} and an \texttt{Object}, thus a cast to either type woule
+be required to make the code compile. However, the warnings reported by Infer do
+not present such ambiguity as in those cases overloaded methods have different
+numbers of parameters. Additionally, even introducing explicit casts for all
+\texttt{null} arguments does not extinguish the warning. Therefore, I can not
+find a conclusive explaination on the nature of the false positive, however I
+can attest to these instances being indeed false positives by having manually
+verified that the methods in question indeed can accept a null value without
+causing null dereferences.
+
+\subsection{True Positives}
+
+In this section I now cover the warnings that are true positives and thus are
+causes for refactoring.
+
 \end{document}