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)

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 class is an inner class, but does not use its embedded reference to the object which created it.  This reference makes the instances of the class larger, and may keep the reference to the creator object alive longer than necessary.  If possible, the class should be made static.

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.

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.

A public static method returns a reference to a mutable object or an array that is part of the static state of the class. Any code that calls this method can freely modify the underlying array. One fix is to return a copy of the array.

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 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 calls algorithmic operations on a String that was returned from a toString() method. As these methods are for debugging/logging purposes, it shouldn't be the basis of core logic in your code.

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.

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());

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.

Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.

See CWE-173: Improper Handling of Alternate Encoding.

Cryptographic keys should not be kept in the source code. The source code can be widely shared in an enterprise environment, and is certainly shared in open source. To be managed safely, passwords and secret keys should be stored in separate configuration files or keystores. (Hard coded passwords are reported separately by the Hard coded password pattern)

Vulnerable Code:

byte[] key = {1, 2, 3, 4, 5, 6, 7, 8};
SecretKeySpec spec = new SecretKeySpec(key, "AES");
Cipher aes = Cipher.getInstance("AES");
aes.init(Cipher.ENCRYPT_MODE, spec);
return aesCipher.doFinal(secretData);

References
CWE-321: Use of Hard-coded Cryptographic Key

This method uses a hand-typed String literal to specify a Charset encoding. As this class is compiled with JDK 7 (or better), and the charset in question is available as a constant from the java.nio.charset.StandardCharsets class, it is better to use the .name() method of the appropriate StandardCharsets constant.

The method in question doesn't directly support a Charset as a parameter, only a String. Still, instead of specifying something like "UTF-8" (and potentially mistyping it), use StandardCharsets.UTF_8.name().

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 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) { }

Synchronizing on a non-private lock object can lead to security vulnerabilities. Untrusted code can gain access to the lock object, and can exploit the system to execute unsafe operations. In scenarios where a thread synchronizes on the lock and enters the synchronized block, untrusted code can manipulate the lock object, 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. A public lock object can be accessed directly by untrusted code, while protected lock objects expose the lock object to subclasses. Then subclasses can use the lock object for synchronizing with unrelated operations. Generally a package-private lock object can be useful inside a sealed package if used with care, but it is still exposed to all classes in the package. This can be resolved by declaring the lock object as private final, which restricts access to the lock object, providing granularity and enhanced security.

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 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 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 class uses an ordinary set or map collection and uses an enum class as the key type. It is more performant to use the JDK 1.5 EnumSet or EnumMap classes.

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 uses the instanceof operator in a series of if/else statements to differentiate blocks of code based on type. If these types are related by inheritance, it is cleaner to just define a method in the base class, and use overridden methods in these classes.

There is a statement or branch on an exception path that if executed guarantees that a value is null at this point, and that value that is guaranteed to be dereferenced (except on forward paths involving runtime exceptions).

This method stores the return result in a local variable, and then immediately returns the local variable. It would be simpler just to return the value that is assigned to the local variable, directly.

Instead of the following:

public float average(int[] arr) {
    float sum = 0;
    for (int i = 0; i < arr.length; i++) {
        sum += arr[i];
    }
    float ave = sum / arr.length;
    return ave;
}

public float average(int[] arr) {
    float sum = 0;
    for (int i = 0; i < arr.length; i++) {
        sum += arr[i];
    }
    return sum / arr.length; //Change
}