SpotBugs (4.10.5) 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.

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 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.

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.

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.

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')

The behavior of the String(byte[] bytes) and String.getBytes() is undefined if the string cannot be encoded in the platform's default charset. Instead, use the String(byte[] bytes, String encoding) or String.getBytes(String encoding) constructor which accepts the string's encoding as an argument. Be sure to specify the encoding used for the user's locale.

As per the Java specifications, "UTF-8", "US-ASCII", "UTF-16" and "ISO-8859-1" will all be valid encoding charsets. If you aren't sure, try "UTF-8".

New in Java 1.7, you can specify an encoding from StandardCharsets, like StandardCharsets.UTF_8. These are generally preferrable because you don't have to deal with UnsupportedEncodingException.

This method calls an instance method passing the object that the method is called on as a parameter, such as foo.someMethod(foo); As you already have access to this object thru this, you don't need to pass it.

This method checks a reference for null just before seeing if the reference is an instanceof some class. Since instanceof will return false for null references, the null check is not needed.

This method returns an array that was allocated but apparently not initialized. It is possible that the caller of this method will do the work of initializing this array, but that is not a common pattern, and it is assumed that this array has just been forgotten to be initialized.

This class defines a compareTo(...) method but inherits its equals() method from java.lang.Object. Generally, the value of compareTo should return zero if and only if equals returns true. If this is violated, weird and unpredictable failures will occur in classes such as PriorityQueue. In Java 5 the PriorityQueue.remove method uses the compareTo method, while in Java 6 it uses the equals method.

From the JavaDoc for the compareTo method in the Comparable interface:

It is strongly recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y)). Generally speaking, any class that implements the Comparable interface and violates this condition should clearly indicate this fact. The recommended language is "Note: this class has a natural ordering that is inconsistent with equals."

This method makes extensive use of methods from another class over methods of its own class. Typically this means that the functionality that is accomplished by this method most likely belongs with the class that is being used so liberally. Consider refactoring this method to be contained in that class, and to accept all the parameters needed in the method signature.

This class defines a field or local collection variable with a name that contains a different type of collection in its name. An example would be a Set called userList. This is confusing to the reader, and likely caused by a previous refactor of type, without changing the name. This detector is obviously only checking for English names.

This code creates a java.util.Random object, uses it to generate one random number, and then discards the Random object. This produces mediocre quality random numbers and is inefficient. If possible, rewrite the code so that the Random object is created once and saved, and each time a new random number is required invoke a method on the existing Random object to obtain it.

If it is important that the generated Random numbers not be guessable, you must not create a new Random for each random number; the values are too easily guessable. You should strongly consider using a java.security.SecureRandom instead (and avoid allocating a new SecureRandom for each random number needed).

This method has a high cyclomatic complexity figure, which represents the number of branch points. It is likely difficult to test, and is brittle to change. Consider refactoring this method into several to reduce the risk.

Most ExecutorService objects must be explicitly shut down, otherwise their internal threads can prolong the running of the JVM, even when everything else has stopped.

FindBugs has detected that there are no calls to either the shutdown() or shutdownNow() method, and thus, the ExecutorService is not guaranteed to ever terminate. This is especially problematic for Executors.newFixedThreadPool() and most of the other convenience methods in the Executors class.

Even though there are some exceptions to this, particularly when a custom ThreadFactory is provided, or for ThreadPoolExecutors with allowsCoreThreadTimeOut() set to true, it is good practice to explicitly shutdown the ExecutorService when its utility is done.

This method creates and uses a java.io.FileInputStream or java.io.FileOutputStream object. Unfortunately both of these classes implement a finalize method, which means that objects created will likely hang around until a full garbage collection occurs, which will leave excessive garbage on the heap for longer, and potentially much longer than expected. Java 7 introduced two ways to create streams for reading and writing files that do not have this concern. You should consider switching from these above classes to InputStream is = java.nio.file.Files.newInputStream(myfile.toPath()); OutputStream os = java.nio.file.Files.newOutputStream(myfile.toPath());

Server-Side Request Forgery occur when a web server executes a request to a user supplied destination parameter that is not validated. Such vulnerabilities could allow an attacker to access internal services or to launch attacks from your web server.

URLConnection can be used with file:// protocol or other protocols to access local filesystem and potentially other services.

Vulnerable Code:

new URL(String url).openConnection()

new URL(String url).openStream()

new URL(String url).getContent()

Solution/Countermeasures:

• Don't accept URL destinations from users
• Accept a destination key, and use it to look up the target destination associate with the key
• White list URLs (if possible)
• Validate that the beginning of the URL is part of a white list

References
CWE-918: Server-Side Request Forgery (SSRF)
Understanding Server-Side Request Forgery
CWE-73: External Control of File Name or Path
Abusing jar:// downloads

This method may fail to clean up (close, dispose of) a stream, database object, or other resource requiring an explicit cleanup operation.

In general, if a method opens a stream or other resource, the method should use a try/finally block to ensure that the stream or resource is cleaned up before the method returns.

This bug pattern is essentially the same as the OS_OPEN_STREAM and ODR_OPEN_DATABASE_RESOURCE bug patterns, but is based on a different (and hopefully better) static analysis technique. We are interested is getting feedback about the usefulness of this bug pattern. For sending feedback, check:

• contributing guideline
• mailinglist

In particular, the false-positive suppression heuristics for this bug pattern have not been extensively tuned, so reports about false positives are helpful to us.

See Weimer and Necula, Finding and Preventing Run-Time Error Handling Mistakes (PDF), for a description of the analysis technique.

SEI CERT rule OBJ01-J requires that accessibility to fields must be limited. Otherwise, the values of the fields may be manipulated from outside the class, which may be unexpected or undesired behaviour. In general, requiring that no fields are allowed to be public is overkill and unrealistic. Even the rule mentions that final fields may be public. Besides final fields, there may be other usages for public fields: some public fields may serve as "flags" that affect the behavior of the class. Such flag fields are expected to be read by the current instance (or the current class, in case of static fields), but written by others. If a field is both written by the methods of the current instance (or the current class, in case of static fields) and from the outside, the code is suspicious. Consider making these fields private and provide appropriate setters, if necessary. Please note that constructors, initializers and finalizers are exceptions, if only they write the field inside the class, the field is not considered as written by the class itself.

See CWE-766: Critical Data Element Declared Public.

This field is never read.  The field is public or protected, so perhaps it is intended to be used with classes not seen as part of the analysis. If not, consider removing it from the class.

See CWE-563: Assignment to Variable without Use.

This method uses specific collection interface/classes for parameters when only methods defined in the Collection class 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 printList(List<String> list) {
    for (String s : list) {
        System.out.println(s);
    }
}

private void printList(Collection<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 calls a var arg method, and passes an explicit null value as the var arg parameter. It is better to just not pass any value at all, as there is confusion as to whether the null represents the entire vararg array, or the first element of the varargs array

The variable referenced at this point is known to be null due to an earlier check against null. Although this is valid, it might be a mistake (perhaps you intended to refer to a different variable, or perhaps the earlier check to see if the variable is null should have been a check to see if it was non-null).

This method performs synchronization on an object that is an instance of a class from the java.util.concurrent package (or its subclasses). Instances of these classes have their own concurrency control mechanisms that are orthogonal to the synchronization provided by the Java keyword synchronized. For example, synchronizing on an AtomicBoolean will not prevent other threads from modifying the AtomicBoolean.

Such code may be correct, but should be carefully reviewed and documented, and may confuse people who have to maintain the code at a later date.

According to SEI Cert rule ERR08-J NullPointerException should not be caught. Handling NullPointerException is considered an inferior alternative to null-checking.

This non-compliant code catches a NullPointerException to see if an incoming parameter is null:

boolean hasSpace(String m) {
  try {
    String ms[] = m.split(" ");
    return names.length != 1;
  } catch (NullPointerException e) {
    return false;
  }
}

A compliant solution would use a null-check as in the following example:

boolean hasSpace(String m) {
    if (m == null) return false;
    String ms[] = m.split(" ");
    return names.length != 1;
}

See CWE-395: Use of NullPointerException Catch to Detect NULL Pointer Dereference.

This method catches an exception, and throws a different exception, without incorporating the original exception. Doing so hides the original source of the exception, making debugging and fixing these problems difficult. It is better to use the constructor of this new exception that takes an original exception so that this detail can be passed along to the user. If this exception has no constructor that takes an initial cause parameter, use the initCause method to initialize it instead.

catch (IOException e) {
    throw new MySpecialException("Failed to open configuration", e);
}

Classes that throw exceptions in their constructors are vulnerable to Finalizer attacks

A finalizer attack can be prevented, by declaring the class final, using an empty finalizer declared as final, or by a clever use of a private constructor.

See SEI CERT Rule OBJ-11 for more information.

This method makes two consecutive calls to the same method, using the same constant parameters, on the same instance, without any intervening changes to the objects. If this method does not make changes to the object, which it appears it doesn't, then making two calls is just a waste. These method calls could be combined by assigning the result into a temporary variable, and using the variable the second time.

This class defines a field with the same name as a visible instance field in a superclass. This is confusing, and may indicate an error if methods update or access one of the fields when they wanted the other.

This field is never written.  All reads of it will return the default value. Check for errors (should it have been initialized?), or remove it if it is useless.

See CWE-457: Use of Uninitialized Variable.

This method calls the size() method on a collection and compares the result to zero to see if the collection is empty. For better code clarity, it is better to just use col.isEmpty() or !col.isEmpty().

The highlighted API is used to execute a system command. If unfiltered input is passed to this API, it can lead to arbitrary command execution.

Vulnerable Code:

import java.lang.Runtime;

Runtime r = Runtime.getRuntime();
r.exec("/bin/sh -c some_tool" + input);

References
OWASP: Command Injection
OWASP: Top 10 2013-A1-Injection
CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')

This class implements the Cloneable interface but defines its clone method to return an Object. Since most likely users of this method will need to cast it to the real type, this will be more painful than necessary. Just declare the return value to be the type of this class.

This method's exception signature is constrained by an interface or superclass not to throw any checked exceptions. Therefore a caught checked exception was converted to an unchecked exception and thrown. However, it appears that the class in question is owned by the same author as the constraining interface or superclass. Consider changing the signature of this method to include the checked exception.

The fields of this class appear to be accessed inconsistently with respect to synchronization.  This bug report indicates that the bug pattern detector judged that

• The class contains a mix of locked and unlocked accesses,
• The class is not annotated as javax.annotation.concurrent.NotThreadSafe,
• At least one locked access was performed by one of the class's own methods, and
• The number of unsynchronized field accesses (reads and writes) was no more than one third of all accesses, with writes being weighed twice as high as reads

A typical bug matching this bug pattern is forgetting to synchronize one of the methods in a class that is intended to be thread-safe.

You can select the nodes labeled "Unsynchronized access" to show the code locations where the detector believed that a field was accessed without synchronization.

Note that there are various sources of inaccuracy in this detector; for example, the detector cannot statically detect all situations in which a lock is held.  Also, even when the detector is accurate in distinguishing locked vs. unlocked accesses, the code in question may still be correct.

See CWE-366: Race Condition within a Thread.

The method creates an IO stream object, does not assign it to any fields, pass it to other methods that might close it, or return it, and does not appear to close the stream on all paths out of the method.  This may result in a file descriptor leak.  It is generally a good idea to use a finally block to ensure that streams are closed.

See CWE-459: Incomplete Cleanup.