SpotBugs (4.9.8) Analysis for lib (spotbugsDebug)

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 code stores a reference to an externally mutable object into the internal representation of the object.  If instances are accessed by untrusted code, and 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.

See CWE-374: Passing Mutable Objects to an Untrusted Method.

This method accesses the value of a Map entry, using a key that was retrieved from a keySet iterator. It is more efficient to use an iterator on the entrySet of the map, to avoid the Map.get(key) lookup.

This method prints a stack trace to the console. This is non configurable, and causes an application to look unprofessional. Switch to using loggers so that users can control what is logged and where.

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 method casts the right hand side of an expression to a class that is more specific than the variable on the left hand side of the assignment. The cast only has to be as specific as the variable that is on the left. Using a more specific type on the right hand side just increases cohesion.

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

Methods should be verbs, in mixed case with the first letter lowercase, with the first letter of each internal word capitalized.

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.

Returning a reference to a mutable object value stored in one of the object's fields exposes the internal representation of the object.  If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Returning a new copy of the object is better approach in many situations.

See CWE-374: Passing Mutable Objects to an Untrusted Method.

This method checks to see if an element is not in a set before adding it. This is unnecessary as you can just add the item, and if the item exists, it won't add it, otherwise it will.

As an example, instead of using

                Set mySet = getSomeSet();
                if (!mySet.contains("foo")) {
                    mySet.add("foo");
                }
            

                Set mySet = getSomeSet();
                if (mySet.add("foo")) {
                }
            

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 is using singleton design pattern and has non-private constructor (please note that a default constructor might exist which is not private). Given that, it is possible to create a copy of the object, thus violating the singleton pattern.
The easier solution would be making the constructor private.

For more information, see: SEI CERT MSC07-J, and CWE-543: Use of Singleton Pattern Without Synchronization in a Multithreaded Context.

This call to a generic collection method contains an argument with an incompatible class from that of the collection's parameter (i.e., the type of the argument is neither a supertype nor a subtype of the corresponding generic type argument). Therefore, it is unlikely that the collection contains any objects that are equal to the method argument used here. Most likely, the wrong value is being passed to the method.

In general, instances of two unrelated classes are not equal. For example, if the Foo and Bar classes are not related by subtyping, then an instance of Foo should not be equal to an instance of Bar. Among other issues, doing so will likely result in an equals method that is not symmetrical. For example, if you define the Foo class so that a Foo can be equal to a String, your equals method isn't symmetrical since a String can only be equal to a String.

In rare cases, people do define nonsymmetrical equals methods and still manage to make their code work. Although none of the APIs document or guarantee it, it is typically the case that if you check if a Collection<String> contains a Foo, the equals method of argument (e.g., the equals method of the Foo class) used to perform the equality checks.

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.

A call to notify() or notifyAll() was made without any (apparent) accompanying modification to mutable object state.  In general, calling a notify method on a monitor is done because some condition another thread is waiting for has become true.  However, for the condition to be meaningful, it must involve a heap object that is visible to both threads.

This bug does not necessarily indicate an error, since the change to mutable object state may have taken place in a method which then called the method containing the notification.

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

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

This method defines an anonymous lambda function to be called to fetch a single value from the passed in value. While this will work, it is needlessly complex as this function merely calls a single getter method on the object, and thus the code can be simplied by just passing in a method reference instead.

Instead of

        baubles.stream().map(b -> b.getName()).collect(Collectors.toSet())
        

        baubles.stream().map(b -> magic.getSpell(b)).collect(Collectors.toSet())
        

The use of a predictable random value can lead to vulnerabilities when used in certain security critical contexts. For example, when the value is used as:

• a CSRF token: a predictable token can lead to a CSRF attack as an attacker will know the value of the token
• a password reset token (sent by email): a predictable password token can lead to an account takeover, since an attacker will guess the URL of the "change password" form
• any other secret value

A quick fix could be to replace the use of java.util.Random with something stronger, such as java.security.SecureRandom.

Vulnerable Code:

String generateSecretToken() {
    Random r = new Random();
    return Long.toHexString(r.nextLong());
}

Solution:

import org.apache.commons.codec.binary.Hex;

String generateSecretToken() {
    SecureRandom secRandom = new SecureRandom();

    byte[] result = new byte[32];
    secRandom.nextBytes(result);
    return Hex.encodeHexString(result);
}

References
Cracking Random Number Generators - Part 1 (https://jazzy.id.au)
CERT: MSC02-J. Generate strong random numbers
CWE-330: Use of Insufficiently Random Values
Predicting Struts CSRF Token (Example of real-life vulnerability and exploitation)

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 concatenates the output of a toString() call into a StringBuffer or StringBuilder. It is simpler just to pass the object you want to append to the append call, as that form does not suffer the potential for NullPointerExceptions, and is easier to read.

Keep in mind that Java compiles simple String concatenation to use StringBuilders, so you may see this bug even when you don't use StringBuilders explicitly.

Instead of:

StringBuilder builder = ...;
builder.append(someObj.toString());
...
System.out.println("Problem with the object :" + someObj.toString());

StringBuilder builder = ...
builder.append(someObj);
...
System.out.println("Problem with the object :" + someObj);

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.

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

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.

This class overrides equals(Object), but does not override hashCode(), and inherits the implementation of hashCode() from java.lang.Object (which returns the identity hash code, an arbitrary value assigned to the object by the VM).  Therefore, the class is very likely to violate the invariant that equal objects must have equal hashcodes.

If you don't think instances of this class will ever be inserted into a HashMap/HashTable, the recommended hashCode implementation to use is:

public int hashCode() {
    assert false : "hashCode not designed";
    return 42; // any arbitrary constant will do
}

See CWE-581: Object Model Violation: Just One of Equals and Hashcode Defined.

This serializable class defines a serialVersionUID that appears to be a computed value, however the value does not match the computed value, and thus losses it's value as version indicator. Either create a custom value like 1, 2, 3, 4.. etc, or recompute the serialVersionUID using your IDE.