diff --git a/after/src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java b/after/src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java index ce62340..a1a4ad7 100644 --- a/after/src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java +++ b/after/src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java @@ -473,11 +473,10 @@ public class MethodUtils { // 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. diff --git a/before/infer-out/.global.tenv b/before/infer-out/.global.tenv index f2eb2de..5bfcb6a 100644 Binary files a/before/infer-out/.global.tenv and b/before/infer-out/.global.tenv differ diff --git a/before/infer-out/.infer_runstate.json b/before/infer-out/.infer_runstate.json index 5dc8b3a..181e255 100644 --- a/before/infer-out/.infer_runstate.json +++ b/before/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} \ No newline at end of file +{"run_sequence":[{"date":"2023-04-12 21:56:44.596744+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} \ No newline at end of file diff --git a/before/infer-out/results.db b/before/infer-out/results.db index 5008c55..63e7d62 100644 Binary files a/before/infer-out/results.db and b/before/infer-out/results.db differ diff --git a/before/infer-out/results.db-shm b/before/infer-out/results.db-shm index 7f7726d..895a9c6 100644 Binary files a/before/infer-out/results.db-shm and b/before/infer-out/results.db-shm differ diff --git a/before/infer-out/results.db-wal b/before/infer-out/results.db-wal index 6105161..283f5f2 100644 Binary files a/before/infer-out/results.db-wal and b/before/infer-out/results.db-wal differ diff --git a/report.pdf b/report.pdf index 4df4612..8ea1723 100644 Binary files a/report.pdf and b/report.pdf differ diff --git a/report.tex b/report.tex index 44d0111..727792e 100644 --- a/report.tex +++ b/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> 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}