report done

after/src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java

@@ -473,11 +473,10 @@ static Object[] getVarArgs(final Object[] args, final Class<?>[] methodParameter
 
         // Construct a new array for the variadic parameters
         final Class<?> varArgComponentType = methodParameterTypes[methodParameterTypes.length - 1].getComponentType();
-        final int varArgLength = args.length - methodParameterTypes.length + 1;
-
         if (varArgComponentType == null) {
             throw new IllegalArgumentException("last method parameter type is not an array");
         }
+        final int varArgLength = args.length - methodParameterTypes.length + 1;
 
         Object varArgsArray = Array.newInstance(ClassUtils.primitiveToWrapper(varArgComponentType), varArgLength);
         // Copy the variadic arguments into the varargs array.

report.tex

@@ -159,25 +159,24 @@ result.
 \textbf{File} & \textbf{Line} & \textbf{Kind} & \textbf{True Pos.} &
 \textbf{Reason why flagged expression is a false positive} \\ \midrule
 AnnotationUtils.java & 72 & Null & Yes & \\
-reflect/FieldUtils.java & 126 & Null & Yes & \\
+reflect/MethodUtils.java & 486 & Null & Yes & \\
 concurrent/MultiBackgroundInitializer.java & 160 & Thread Safety & Yes & \\
 \midrule
 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 & \multirow{3}{6.2cm}{The method which
+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 non-null parameter is given} \\ 
-reflect/MethodUtils.java & 486 & Null & No & \\
-        \\ \hspace{1cm} \\
+        non-null, and a non-null parameter is given \\ 
 \midrule 
 CharSetUtils.java & 181 & Null & No & According to \textit{java.lang}
 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
+reflect/FieldUtils.java & 126 & Null & No & 
+\multirow{5}{6cm}{A utility method is used to guard the dereference reported
 with an exception throw} \\
+reflect/FieldUtils.java & 341 & Null & No & \\
 reflect/FieldUtils.java & 385 & Null & No & \\
 reflect/FieldUtils.java & 599 & Null & No & \\
 reflect/FieldUtils.java & 644 & Null & No & \\
@@ -279,37 +278,199 @@ causing null dereferences.
 In this section I now cover the warnings that are true positives and thus are
 causes for refactoring.
 
-\begin{center}
-\color{red}
-- field utils violation of contract
+A copy of the entire code base where refactoring has been applied can be found
+in the \textit{after} directory of the assignment's repository.
 
-- missing synchronized in multibackground
+\begin{listing}[H]
+\begin{minted}[linenos,firstnumber=71]{java}
+@Override
+protected String getShortClassName(final Class<?> cls) {
+    for (final Class<?> iface : ClassUtils.getAllInterfaces(cls)) {
+        if (Annotation.class.isAssignableFrom(iface)) {
+            return "@" + iface.getName();
+        }
+    }
+    return StringUtils.EMPTY;
+}
+\end{minted}
+    \caption{Method \textit{getShortClassName(Class<?>)} of class
+    \textit{AnnotationUtils} in Apache Commons Lang 3.12.0.}
+    \label{lst:ann}
+\end{listing}
 
-- Annotation utils protected method no contract on field fixed by returning
-"empty" value
+Listing \ref{lst:ann} shows the location of the first true positive warning.
+Here, \mintinline{java}{ClassUtils.getAllInterfaces(cls)} at line 73 may
+return a null value thus potentially causing a null dereference when iterating
+in the enhanced for loop. Indeed, the method's implementation shows that while
+for non-null \texttt{cls} values the return value is non-null, if \texttt{cls}
+is null \texttt{null} is returned. Since the method is protected its signature
+is visible to all child classes, even ones potentially outside of the Apache
+Commons Lang library. As the method's documentation does not specify anything
+about the nullability of the argument \texttt{cls}, its value may indeed be null
+and thus a refactor is needed to avoid the potential null dereference.
 
-- no additional warnings appear after fix
+I choose to simply return \mintinline{java}{StringUtils.EMPTY} when \texttt{cls}
+is null. The refactored implementation can be found in listing \ref{lst:ann2}.
 
-- cool that a contract violation has been found
+\begin{listing}[H]
+\begin{minted}[linenos,firstnumber=71]{java}
+@Override
+protected String getShortClassName(final Class<?> cls) {
+    final List<Class<?>> interfaces = ClassUtils.getAllInterfaces(cls);
+    if (interfaces == null) {
+        return StringUtils.EMPTY;
+    }
 
-- tests pass after fixes so no regression
-\end{center}
+    for (final Class<?> iface : interfaces) {
+        if (Annotation.class.isAssignableFrom(iface)) {
+            return "@" + iface.getName();
+        }
+    }
+    return StringUtils.EMPTY;
+}
+\end{minted}
+    \caption{Refactor of method \textit{getShortClassName(Class$<$?$>$)} of
+    class \textit{AnnotationUtils}.}
+    \label{lst:ann2}
+\end{listing}
+
+Next, class \textit{reflect.MethodUtils} contains a bug due to insufficient
+defensiveness when accepting the method parameters.
+
+\begin{listing}[H]
+    \small
+\begin{minted}[linenos,firstnumber=461,breaklines]{java}
+static Object[] getVarArgs(final Object[] args, final Class<?>[] methodParameterTypes) {
+    if (args.length == methodParameterTypes.length && (args[args.length - 1] == null ||
+            args[args.length - 1].getClass().equals(methodParameterTypes[methodParameterTypes.length - 1]))) {
+        // The args array is already in the canonical form for the method.
+        return args;
+    }
+
+    // Construct a new array matching the method's declared parameter types.
+    final Object[] newArgs = new Object[methodParameterTypes.length];
+
+    // Copy the normal (non-varargs) parameters
+    System.arraycopy(args, 0, newArgs, 0, methodParameterTypes.length - 1);
+
+    // Construct a new array for the variadic parameters
+    final Class<?> varArgComponentType = methodParameterTypes[methodParameterTypes.length - 1].getComponentType();
+    final int varArgLength = args.length - methodParameterTypes.length + 1;
+
+    Object varArgsArray = Array.newInstance(ClassUtils.primitiveToWrapper(varArgComponentType), varArgLength);
+    // Copy the variadic arguments into the varargs array.
+    System.arraycopy(args, methodParameterTypes.length - 1, varArgsArray, 0, varArgLength);
+
+    if (varArgComponentType.isPrimitive()) {
+        // unbox from wrapper type to primitive type
+        varArgsArray = ArrayUtils.toPrimitive(varArgsArray);
+    }
+
+    // Store the varargs array in the last position of the array to return
+    newArgs[methodParameterTypes.length - 1] = varArgsArray;
+
+    // Return the canonical varargs array.
+    return newArgs;
+}
+\end{minted}
+    \caption{Method \textit{getVarArgs(Object[], Class$<$?$>$[])} of class
+    \textit{reflect.MethodUtils} in Apache Commons Lang 3.12.0.}
+    \label{lst:met}
+\end{listing}
+
+Line 482 of the class (listed in listing \ref{lst:met}) is correctly flagged as
+a potential null dereference as \texttt{varArgComponentType} may be null.
+Indeed, according to the documentation of the method generating its value, the
+variable is null when the last value in the array \texttt{methodParameterTypes}
+is a class object for a non-array class.  Such value would violate the method's
+contract, which specifies that the array should correspond to class types for
+the arguments of a variadic method, and the last argument in a variadic method
+is always an array. However, this is not checked defensively, and we can fix
+this by introducing a check after line 475 like this:
+
+\begin{minted}{java}
+if (varArgComponentType == null) {
+    throw new IllegalArgumentException("last method parameter type is not an array");
+}
+\end{minted}
+
+It is quite interesting Infer was able to detect this null dereference as it
+stems from a lack of sufficient defensiveness. Improper use of the method would
+trigger a \textit{NullPointerException} without any useful message indicating
+the precondition violation.
+
+Finally, the last warning involves class
+\textit{concurrent.MultiBackgroundInitializer}. The relevant method source code
+is shown in listing \ref{lst:mbi}.
+
+\begin{listing}[H]
+\begin{minted}[linenos,firstnumber=157]{java}
+@Override
+protected int getTaskCount() {
+    int result = 1;
+
+    for (final BackgroundInitializer<?> bi : childInitializers.values()) {
+        result += bi.getTaskCount();
+    }
+
+    return result;
+}
+\end{minted}
+    \caption{Method \textit{getVarArgs(Object[], Class$<$?$>$[])} of class
+    \textit{reflect.MethodUtils} in Apache Commons Lang 3.12.0.}
+    \label{lst:met}
+\end{listing}
+
+Here, the object pointed by \texttt{childInitializers} is accessed without using
+any lock, while other methods of the class use the implicit lock of the instance
+(i.e. \mintinline{java}{synchronized(this)}). Simply adding a
+\mintinline{java}{synchronized} block around the for loop extinguishes the
+warning.
+
+The refactoring performed for all three warnings has not produced further
+warnings, and it has not caused the failure of any unit test.
 
 \section{Conclusions}
 
-\begin{center}
-\color{red}
-- project is mature, few detections but still a couple for null values which
-    shows how insidious they are. (Null and thread safety are popular and
-    relevant to the project) 
+Given that Apache Commons Lang is a very mature project (the code still contains
+workarounds targeting Java 1.3) it is not surprising that Infer has detected a
+relatively limited number of warnings. If anything this should be a good
+testament to the skills and discipline of all project contributors. However, 3
+true positives were found in this production-ready release version. This is a
+testament to the insidiousness of some programming mistakes like lack of
+defensiveness when dealing with possibly null values and improper use of lock
+constructs, a fact which applies to this project as well.
 
-- infer compared to pmd gives warnings about potential execution paths (data
-flow analysis) while pmd is style only and bound to the language
+The high false positive ratio of the analysis highlights the tradeoff between
+soundness and completeness software analysis tecniques face. Given that the true
+positives were quite non-trivial, nevertheless Infer proves itself as a useful
+tool even with mature and production-ready software.
 
-- sonarqube kind of covers some ground infer does. it combines the powers of
-pmd (patterns common to the language to avoid/pursue) with some data flow 
-analysis (some nullability check). No overlap with Infer results
-\end{center}
+To draw some conclusions between this analysis and the one performed last
+semester during the SDM class, I hereby compare the current results with the
+ones formerly found.
+
+When compared to the PMD source code analyzer and SonarQube, Infer reports
+issues closer to external quality factors (i.e. user satisfaction, and
+consequently runtime behaviour) rather than internal quality ones, such as the
+adherence to code style rules and presence of adequate documentation. 
+
+Some
+overlap is present in the scope of the analysis of both tools, namely with
+respect to thread safety (and null dereference with SonarQube). It is worth to
+note that all three tool reported warnings with distinct locations in the source
+code, and no overlap was found between them.
+
+However, due to
+its control and data flow analysis capabilities, Infer is a superior tool to
+understand and check possible runtime behavior while analyzing only the source
+code.
+
+We can also say both PMD and SonarQube have superior awareness regarding the
+behaviour of Java and its standard library. For example, they are aware of
+boxing and unboxing of primitive types (and SonarQube even produces warnings
+about misuse of the feature), where as hinted by this analysis Infer seems not
+to be aware of, at least while searching null dereferences.
 
 \end{document}