SpotBugs (4.10.4) Analysis for app (spotbugsGplayDebug)

This method allocates a collection using the default constructor even though it is known a priori (or at least can be reasonably guessed) how many items are going to be placed in the collection, and thus needlessly causes intermediate reallocations of the collection.

You can use the constructor that takes an initial size and that will be much better, but due to the loadFactor of Maps and Sets, even this will not be a correct estimate.

If you are using Guava, use its methods that allocate maps and sets with a predetermined size, to get the best chance for no reallocations, such as:

• Sets.newHashSetWithExpectedSize(int)
• Maps.newHashMapWithExpectedSize(int)

This class defines static fields that are Collections, StringBuffers, or StringBuilders that do not appear to have any way to clear or reduce their size. That is, a collection is defined and has method calls like
{add(), append(), offer(), put(), ...}
with no method calls to removal methods like
{clear(), delete(), pop(), remove(), ...}
This means that the collection in question can only ever increase in size, which is a potential cause of memory bloat.

If this collection is a list, set or otherwise of static things (e.g. a List<String> for month names), consider adding all of the elements in a static initializer, which can only be called once:

private static List<String> monthNames = new ArrayList<String>();
static {
    monthNames.add("January");
    monthNames.add("February");
    monthNames.add("March");
    ...
}

When using the read() method to fill an array, the method may not always fill the entire array. This can occur when the input stream does not have enough bytes available to completely fill the array, leading to a partial read. This issue can result in unexpected behavior if the partially filled array is assumed to be fully populated.

It is crucial to verify the return value of the read() method, which indicates the number of bytes actually read, and to ensure that the array is fully filled if necessary. Failing to do so can result in incomplete data processing and potential vulnerabilities.

To fix this issue, check the return value of read() and handle cases where fewer bytes are read than expected. Consider looping over the read() call or using methods like readFully() to ensure that the array is filled completely.

See SEI CERT rule FIO10-J. Ensure the array is filled when using read() to fill an array for more information.

Static synchronized methods use the class object as a monitor (that is, its intrinsic lock), thus an attacker, if it has access to an instance of the class or the subclass, can gain access to the class object using getClass() and indefinitely lock the object, leading to a Denial of Service (DoS) scenario. This can be resolved in two ways: either by using a private static final lock object with block synchronization or by restricting access to the class. Using a private final lock object is generally preferred, as it offers enhanced security and granularity of control.

For more details read SEI CERT rule LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code.

This method accesses an array or list using a constant integer index. Often, this is a typo where a loop variable is intended to be used. If however, specific list indices mean different specific things, then perhaps replacing the list with a first-class object with meaningful accessors would make the code less brittle.

This class creates and initializes a collection as a field but then never accesses this collection to gain information or fetch items from the collection. It is likely that this collection is left over from a past effort, and can be removed.

Concatenating unvalidated user input into a URL can allow an attacker to override the value of a request parameter. Attacker may be able to override existing parameter values, inject a new parameter or exploit variables out of a direct reach. HTTP Parameter Pollution (HPP) attacks consist of injecting encoded query string delimiters into other existing parameters. If a web application does not properly sanitize the user input, a malicious user may compromise the logic of the application to perform either client-side or server-side attacks.
In the following example the programmer has not considered the possibility that an attacker could provide a parameter lang such as en&user_id=1, which would enable him to change the user_id at will.

Vulnerable Code:

String input = request.getParameter("lang");
GetMethod get = new GetMethod("http://www.host.com/viewDetails");
get.setQueryString("lang=" + input + "&user_id=" + userId);
get.execute();

Solution:
You can either encode user input before placing it in HTTP parameters or use the UriBuilder class from Apache HttpClient.

URIBuilder uriBuilder = new URIBuilder("http://www.host.com/viewDetails");
uriBuilder.addParameter("lang", input);
uriBuilder.addParameter("user_id", userId);

HttpGet httpget = new HttpGet(uriBuilder.build().toString()); //OK

References
CAPEC-460: HTTP Parameter Pollution (HPP) CWE-235: Improper Handling of Extra Parameters

The return value from a method is dereferenced without a null check, and the return value of that method is one that should generally be checked for null. This may lead to a NullPointerException when the code is executed.

This method uses an integer-based for loop to iterate over a java.util.List, by calling List.get(i) each time through the loop. The integer is not used for other reasons. It is better to use an Iterator instead, as depending on List implementation, iterators can perform better, and they also allow for exchanging of other collection types without issue.

This method sets the value of an enum reference to null. An enum should never have a null value. If there is a state where you do not know what the value of an enum should be, than that should be one of the proper enum values. So add a MyEnum.UNKNOWN or such. This keeps the logic of switch statements, etc, much simpler.

This method uses a try-catch block that catches Exception objects, but Exception is not thrown within the try block, and RuntimeException is not explicitly caught. It is a common bug pattern to say try { ... } catch (Exception e) { something } as a shorthand for catching a number of types of exception each of whose catch blocks is identical, but this construct also accidentally catches RuntimeException as well, masking potential bugs.

A better approach is to either explicitly catch the specific exceptions that are thrown, or to explicitly catch RuntimeException exception, rethrow it, and then catch all non-Runtime Exceptions, as shown below:

try {
    ...
} catch (RuntimeException e) {
    throw e;
} catch (Exception e) {
    ... deal with all non-runtime exceptions ...
}

See CWE-396: Declaration of Catch for Generic Exception.

This class declares that it implements an interface, but does so by relying on methods supplied by superclasses, even though those superclasses know nothing about the interface in question. If you wish to have the child not implement all the methods of the interface, it would probably be better to declare the superclass as implementing the interface, and if that class does not provide all the methods, then declare that superclass abstract.

This method contains a redundant check of a known non-null value against the constant null.

A value is checked here to see whether it is null, but this value cannot be null because it was previously dereferenced and if it were null a null pointer exception would have occurred at the earlier dereference. Essentially, this code and the previous dereference disagree whether this value is allowed to be null. Either the check is redundant or the previous dereference is erroneous.

This instance method writes to a static field. This is tricky to get correct if multiple instances are being manipulated, and generally bad practice.

This class appears to maintain two or more lists or arrays whose contents are related in a parallel way. That is, you have something like:

List<String> words = new ArrayList<String>();
List<Integer> wordCounts = new ArrayList<String>();

Consider creating a separate class to hold all the related pieces of information, and adding instances of this class to just one list or array, or if just two values, use a Map to associate one value with the other like:

private class WordAndCount{public String word;  public int count}

List<WordAndCount> wordsAndCounts = new ArrayList<WordAndCount>();
//or, for just two elements
Map wordCounts = new HashMap();

This method uses concrete classes for parameters when only methods defined in an implemented interface or superclass are used. Consider increasing the abstraction of the interface to make low impact changes easier to accomplish in the future.

Take the following example:

private void appendToList(ArrayList<String> list) {
    if (list.size() < 100) {
        list.add("Foo");
    }
}

private void appendToList(List<String> list) {
    ...

IDEs tend to have tools to help generalize parameters. For example, in Eclipse, the refactoring tool Generalize Declared Type helps find an appropriate level of concreteness.

This method defines parameters that are never used. As this method is either static or private, and can't be derived from, it is safe to remove these parameters and simplify your method. You should consider, while unlikely, that this method may be used reflectively, and thus you will want to change that call as well. In this case, it is likely that once you remove the parameter, there will be a chain of method calls that have spent time creating this parameter and passing it down the line. All of this may be able to be removed.

This class 'overloads' the same method with both instance and static versions. As the use of these two models is different, it will be confusing to the users of these methods.

Synchronized methods use the class object as a monitor (that is, its intrinsic lock), thus an attacker, if it has access to the object, can exploit the system by indefinitely locking the object, leading to a Denial of Service (DoS) scenario. This can be resolved in two ways: either by using a private final lock object with block synchronization or by restricting access to the class. Using a private final lock object is generally preferred, as it offers enhanced security and granularity of control.

For more details read SEI CERT rule LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code.

This method appears to modify a parameter, and then return this parameter as the method's return value. This will be confusing to callers of this method, as it won't be apparent that the 'original' passed-in parameter will be changed as well. If the purpose of this method is to change the parameter, it would be more clear to change the method to have a void return value. If a return type is required due to interface or superclass contract, perhaps a clone of the parameter should be made.

This method creates an array initialized by constants. Each time this method is called this array will be recreated. It would be more performant to define the array as a static field of the class instead.

Synchronizing on a lock object that is made accessible to outside classes by returning it from a method or updating it through a method can lead to security vulnerabilities. Untrusted code can gain access to the lock or modify its value and exploit the system. In scenarios where a thread synchronizes on the lock and enters the synchronized block, an attacker can access the lock object or update its value, allowing another thread to synchronize on the new value and concurrently enter the synchronized block. Alternatively, an attacker can indefinitely hold the lock object, resulting in a Denial of Service (DoS) scenario. This can be resolved in two ways: either by using another object as a private final lock object, or by removing or restricting the visibility of the functions, which update or return the lock object. Using a private final lock object is generally preferred, as it offers enhanced security and granularity of control.

For more details read SEI CERT rule LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code.

This method returns or throws exceptions from a finally block. This will mask real program logic in the try block, and short-circuit normal method termination.

This class implements the Comparator interface. You should consider whether or not it should also implement the Serializable interface. If a comparator is used to construct an ordered collection such as a TreeMap, then the TreeMap will be serializable only if the comparator is also serializable. As most comparators have little or no state, making them serializable is generally easy and good defensive programming.

See CWE-1066: Missing Serialization Control Element.

This method contains a switch statement where default case is missing. Usually you need to provide a default case.

Because the analysis only looks at the generated bytecode, this warning can be incorrect triggered if the default case is at the end of the switch statement and the switch statement doesn't contain break statements for other cases.

See CWE-478: Missing Default Case in Multiple Condition Expression.

This instanceof test will always return true (unless the value being tested is null). Although this is safe, make sure it isn't an indication of some misunderstanding or some other logic error. If you really want to test the value for being null, perhaps it would be clearer to do better to do a null test rather than an instanceof test.

See CWE-571: Expression is Always True.

This method builds a conditional expression, for example, in an if or while statement, where the expressions contain both simple local variable comparisons and comparisons on method calls. The expression orders these so that the method calls come before the simple local variable comparisons. This causes method calls to be executed in conditions when they do not need to be, and thus potentially causes a lot of code to be executed for nothing. By ordering the expressions so that the simple conditions containing local variable conditions are first, you eliminate this waste. This assumes that the method calls do not have side effects. If the methods do have side effects, it is probably a better idea to pull these calls out of the condition and execute them first, assigning a value to a local variable. In this way you give a hint that the call may have side effects.

Example:

if ((calculateHaltingProbability() > 0) && shouldCalcHalting) { }

if (shouldCalcHalting && (calculateHaltingProbability() > 0) { }

This method declares a RuntimeException derived class in its throws clause. This may indicate a misunderstanding as to how unchecked exceptions are handled. If it is felt that a RuntimeException is so prevalent that it should be declared for documentation purposes, using javadoc's @throws clause is a better idea, as it doesn't needless pollute the method signature,

This method declares that it returns a Collection, when in fact it has created and returned a more specific interface. Since the caller is free to cast any return value to Collection, it is better to return the specific interface created so that it gives the caller the most power. With methods, always expect the least with parameters, and provide the most with return types.

This code stores a reference to an externally mutable object into a static field. If unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Storing a copy of the object is better approach in many situations.

This method allocates and uses an auto closeable resource. However, it manually closes the resource in a finally block. While this is correct management, it doesn't rely on the idiomatic way available to JDK 7 and above, allows for possible subtle problems, and complicates the reading of code by developers expecting the use of try-with-resources.

Switch to using try with resources, as:

    		    try (InputStream is = getAStream()) {
    		        useTheStream(is);
    		    }
    		

This constructor makes a call to a non-final method. Since this method can be overridden, a subclass' implementation will be executing against an object that has not been initialized at the subclass level. You should mark all methods called from the constructor as final to avoid this problem.

This method retrieves the property of a Java bean, only to use it in the setter for the same property of the same bean. This is usually a copy/paste typo.

This method creates an object but does not assign this object to any variable or field. This implies that the class operates through side effects in the constructor, which is a bad pattern to use, as it adds unnecessary coupling. Consider pulling the side effect out of the constructor, into a separate method, or into the calling method.

An attacker might be able to detect the value of the secret hash due to the exposure of comparison timing. When the functions Arrays.equals() or String.equals() are called, they will exit earlier if fewer bytes are matched.

Vulnerable Code:

String actualHash = ...

if(userInput.equals(actualHash)) {
    ...
}

Solution:

String actualHash = ...

if(MessageDigest.isEqual(userInput.getBytes(),actualHash.getBytes())) {
    ...
}

References
CWE-203: Information Exposure Through DiscrepancyKey

Object deserialization of untrusted data can lead to remote code execution, if there is a class in classpath that allows the trigger of malicious operation.

Libraries developers tend to fix class that provided potential malicious trigger. There are still classes that are known to trigger Denial of Service^[1].

Deserialization is a sensible operation that has a great history of vulnerabilities. The web application might become vulnerable as soon as a new vulnerability is found in the Java Virtual Machine^{[2] [3]}.

Code at risk:

public UserData deserializeObject(InputStream receivedFile) throws IOException, ClassNotFoundException {

    try (ObjectInputStream in = new ObjectInputStream(receivedFile)) {
        return (UserData) in.readObject();
    }
}

Solutions:

Avoid deserializing object provided by remote users.

References
CWE-502: Deserialization of Untrusted Data
Deserialization of untrusted data
Serialization and Deserialization
A tool for generating payloads that exploit unsafe Java object deserialization
[1] Example of Denial of Service using the class java.util.HashSet
[2] OpenJDK: Deserialization issue in ObjectInputStream.readSerialData() (CVE-2015-2590)
[3] Rapid7: Sun Java Calendar Deserialization Privilege Escalation (CVE-2008-5353)

This method calls a parsing method (indexOf, lastIndexOf, startsWith, endsWith, substring, indexOf) on a String that is a field, or comes from a collection that is a field. This implies that the String in question is holding multiple parts of information inside the string, which would be more maintainable and type safe if that value was a true collection or a first class object with fields, rather than a String.

This parameter is always used in a way that requires it to be non-null, but the parameter is explicitly annotated as being Nullable. Either the use of the parameter or the annotation is wrong.

Looks for relatively large if blocks of code, where you unconditionally return from them, and then follow that with an unconditional return of a small block. This places the bulk of the logic to the right indentation-wise, making it more difficult to read than needed. It would be better to invert the logic of the if block, and immediately return, allowing the bulk of the logic to be move to the left for easier reading.

Suppressing annotations &SuppressFBWarnings should be removed from the source code as soon as they are no more needed. Leaving them may result in accidental warnings suppression. The annotation was probably added to suppress a warning raised by SpotBugs, but now SpotBugs does not report the bug anymore. Either the bug was solved or SpotBugs was updated, and the bug is no longer raised by that code. The unnecessary annotation is not reported when the class or method is annotated with an &Generated annotation with retention CLASS or RUNTIME.

SEI CERT rule VNA00-J describes that reading a shared primitive variable in one thread may not yield the value of the most recent write to the variable from another thread. Consequently, the thread may observe a stale value of the shared variable.

To fix it, declare the variable volatile, change the type of the field to the corresponding atomic type from java.lang.concurrent.atomic or correctly synchronize the code. Declaring the variable volatile may not be enough in some cases: e.g. when the variable is assigned a value which depends on the current value or on the result of nonatomic compound operations. This guarantees that 64-bit primitive long and double variables are accessed atomically.

See also CWE-413: Improper Resource Locking, CWE-567: Unsynchronized Access to Shared Data in a Multithreaded Context, and CWE-667: Improper Locking.

This method passes a constant literal String of length 1 as a parameter to a method, when a similar method is exposed that takes a char. It is simpler and more expedient to handle one character, rather than a String.

Instead of making calls like:

String myString = ...
if (myString.indexOf("e") != -1) {
    int i = myString.lastIndexOf("e");
    System.out.println(myString + ":" + i);  //the Java compiler will use a StringBuilder internally here [builder.append(":")]
    ...
    return myString.replace("m","z");
}

String myString = ...
if (myString.indexOf('e') != -1) {
    int i = myString.lastIndexOf('e');
    System.out.println(myString + ':' + i);  //the Java compiler will use a StringBuilder internally here [builder.append(':')]
    ...
    return myString.replace('m','z');
}

Suppressing annotations &SuppressFBWarnings should be removed from the source code as soon as they are no more needed. Leaving them may result in accidental warnings suppression. The unnecessary annotation is not reported when the class is annotated with an &Generated annotation with retention CLASS or RUNTIME.

The long and the double are 64-bit primitive types, and depending on the Java Virtual Machine implementation assigning a value to them can be treated as two separate 32-bit writes, and as such it's not atomic. See JSL 17.7. Non-Atomic Treatment of double and long. See SEI CERT rule VNA05-J. Ensure atomicity when reading and writing 64-bit values and CWE-667: Improper Locking for more info. This bug can be ignored in platforms which guarantee that 64-bit long and double type read and write operations are atomic.

To fix it, declare the variable volatile, change the type of the field to the corresponding atomic type from java.lang.concurrent.atomic or correctly synchronize the code. Declaring the variable volatile may not be enough in some cases: e.g. when the variable is assigned a value which depends on the current value or on the result of nonatomic compound operations.

This method creates a java.nio.file.Path object by first creating a java.io.File object, and then calling toPath() on it. It is simpler to just construct the Path object directly, say by using {@code Paths.get(String...)}.

This method checks for the presence of a key in a map using containsKey(), before attempting to fetch the value of the key using get(). This equates to doing two map lookups in a row. It is much simpler to just fetch the value with get, and checking for non null instead.

As an example, instead of using

    	    	Map myMap = getSomeMap();
    	    	if (myMap.containsKey("foo")) {
    	    		String value = myMap.get("foo");
    	    		....
    	    	}
    	    

     	    	Map myMap = getSomeMap();
     	    	String value = myMap.get("foo");
    	    	if (value != null) {
    	    		....
    	    	}
    	    

The only caveat to this is that if you use a null value in a map to represent a third state for the key, then in this case using containsKey is 'correct'. This means an entry found in the map with a null value is taken differently than no entry at all. However, this is a very subtle programming paradigm, and likely to cause problems. If you wish to mark an entry as not being present, it is better to use a named 'sentinel' value to denote this, so instead of:

    	    	Map myMap = getSomeMap();
    	    	if (myMap.containsKey("foo")) {
    	    		String value = myMap.get("foo");
    	    		....
    	    	}
    	    

     	    	Map myMap = getSomeMap();
     	    	String value = myMap.get("foo");
    	    	if (NOT_FOUND.equals(value)) {
    	    		....
    	    	}
    	    	where NOT_FOUND is some constant that denotes this special status. Of course you will need to find a special
    	    	sentinel value for each type you are using that isn't possible to have normally.
    	    

This method passes a String to a wrapped primitive object's parse method, which in turn calls the valueOf method to convert to a boxed primitive. When it is desired to convert from a String to a boxed primitive object, it is simpler to use the BoxedPrimitive.valueOf(String) method.

Instead of something like:

Boolean bo = Boolean.valueOf(Boolean.parseBoolean("true"));
Float f = Float.valueOf(Float.parseFloat("1.234"));

Boolean bo = Boolean.valueOf("true");
Float f = Float.valueOf("1.234");

This code calls a method and ignores the return value. However, our analysis shows that the method (including its implementations in subclasses if any) does not produce any effect other than return value. Thus, this call can be removed.

We are trying to reduce the false positives as much as possible, but in some cases this warning might be wrong. Common false-positive cases include:

- The method is designed to be overridden and produce a side effect in other projects which are out of the scope of the analysis.

- The method is called to trigger the class loading which may have a side effect.

- The method is called just to get some exception.

If you feel that our assumption is incorrect, you can use a @CheckReturnValue annotation to instruct SpotBugs that ignoring the return value of this method is acceptable.

This instance field appears to only be set to a literal value in a constructor, and never reset. Thus it is likely that this field could just be defined as a static field instead.

Unsafe strings (e.g. from a user input) may contain XML tags. If such a string is inserted into an XML document then it may change its structure to a valid but semantically different document. To prevent this unsafe strings must b sanitized first.

Example:
An XML document may look like this:

⟨product⟩
    ⟨name⟩Cell Phone⟨/name⟩
    ⟨price⟩800⟨/price⟩
    ⟨amount⟩1⟨/amount⟩
⟨/product⟩

1⟨/amount⟩⟨price⟩1⟨/price⟩⟨amount⟩1

References
IDS16-J. Prevent XML Injection

This method uses the reflective setAccessible method to alter the behavior of methods and fields in classes in ways that were not expected to be accessed by the author. Doing so circumvents the protections that the author provided through the class definition, and may expose your application to unexpected side effects and problems. This functionality is deprecated in Java 9, and in Java 10 it is expected that this functionality won't work at all.

This method is not constrained by an interface or superclass, but converts a caught checked exception to an unchecked exception and throws it. It would be more appropriate just to throw the checked exception, adding the exception to the throws clause of the method.

This method contains a redundant check of a known null value against the constant null.

A file is opened to write to its contents. The filename comes from an input parameter. If an unfiltered parameter is passed to this file API, files at an arbitrary filesystem location could be modified.

This rule identifies potential path traversal vulnerabilities. In many cases, the constructed file path cannot be controlled by the user. If that is the case, the reported instance is a false positive.

References
WASC-33: Path Traversal
OWASP: Path Traversal
CAPEC-126: Path Traversal
CWE-22: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')