#217 Patch to add detectors for more "dumb methods"
This patch includes MoreDumbMethods.java that uses Java method signature strings to easily check for many dumb or deprecated methods. The MoreDumbMethodsTest.java demonstrates all of the new detectors:
* DM_RUNTIME_EXIT_OR_HALT: Calling Runtime.exit() or Runtime.halt() shuts down the entire Java virtual machine. This should only been done when it is appropriate. Such calls make it hard or impossible for your code to be invoked by other code. Consider throwing a RuntimeException instead.
* DM_RUNFINALIZATION: Triggering finalization can result in serious performance problems and may indicate incorrect resource cleanup.
* EQ_BIGDECIMAL_EQUALS: equals() being called to compare two java.math.BigDecimal numbers. This is normally a mistake, as two BigDecimal objects are only equal if they are equal in both value and scale, so that 2.0 is not equal to 2.00. To compare BigDecimal objects for mathematical equality, use compareTo() instead.
* DM_INETADDRESS_GETLOCALHOST: Do not call InetAddress.getLocalHost() on multihomed servers. On a multihomed server, InetAddress.getLocalHost() simply returns the IP address associated with the server's internal hostname. This could any of the network interfaces, which could expose the machine to security risks. Server applications that need to listen on sockets should add configurable properties to define which network interfaces the server should bind.
* DM_PROMISCUOUS_SERVERSOCKET: Do not use the ServerSocket constructor or ServerSocketFactory.createServerSocket() factory methods that accepts connections on any network interface. By default, an application that listens on a socket will listen for connection attempts on any network interface, which can be a security risk. Only the long form the ServerSocket constructor or ServerSocketFactory.createServerSocket() factory methods take a specific local address to define which network interface the socket should bind.
* RV_RANDOM_SEED: Random() constructor without a seed is insecure because it defaults to easily guessable seed: System.currentTimeMillis(). Initialize seed with Random(SecureRandom.getInstance().generateSeed()) or use SecureRandom instead.
* RV_SECURERANDOM: The SecureRandom() constructors and SecureRandom.getSeed() method are deprecated. Call SecureRandom.getInstance() and SecureRandom.getInstance().generateSeed() instead.
* DM_THREAD_PRIORITIES: Getting or setting thread priorities is not portable and could indicate race conditions.
* DM_THREAD_YIELD: Manual thread scheduling with Thread.sleep() or Thread.yield() has no guaranteed semantics and is often used to mask race conditions.
* DM_WAIT_WITHOUT_TIMEOUT: Calling {2} without timeout could block forever. Consider using a timeout to detect deadlocks or performance problems. Thread.join() Object.wait() Condition.await() Lock.lock() Lock.lockInterruptibly()
* DM_THREAD_FAIRNESS: Calling Lock.tryLock() or ReentrantLock.tryLock() without a timeout does not honor the lock's fairness setting. If you want to honor the fairness setting for this lock, then use tryLock(0, TimeUnit.SECONDS) which is almost equivalent (it also detects interruption).
* DM_SIGNAL_NOT_SIGNALALL: Condition.signalAll() is prefered over Condition.signal(). Calling signal() only wakes up one thread, meaning that the thread woken up might not be the one waiting for the condition that the caller just satisfied.
* DM_LOCK_ISLOCKED: Calling ReentrantLock.isLocked() or ReentrantLock.isHeldByCurrentThread() might indicate race conditions or incorrect locking. These methods are designed for use in debug code or monitoring of the system state, not for synchronization control.
* DM_STRINGBUFFER: StringBuffer's methods are synchronized. StringBuilder is faster and preferred if you don't need StringBuffer's thread safety.
* DM_STRING_BYTES_ENCODING: 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.
* DM_SETDEFAULTLOCALE: Do not use the Locale.setDefault() method to change the default locale. It changes the JVM's default locale for all threads and makes your applications unsafe to threads. It does not affect the host locale. Since changing the JVM's default locale may affect many different areas of functionality, this method should only be used if the caller is prepared to reinitialize locale-sensitive code running within the same Java Virtual Machine, such as the user interface.
test file
patch file
Some of these detectors (such as the socket security checks and wait-without-timeout checks) were inspired by recommendations in Michael Nygard's excellent book "Release It!: Design and Deploy Production-Ready Software".
[bump] Any comments or interest in this proposed patch? Thanks.
Please remove DM_STRING_BYTES_ENCODING, as it is already implemented in FB.
For the rest, please rebase the patch on latest git state.
Just a heads up, these detectors are currently implemented in fb-contrib.
https://github.com/mebigfatguy/fb-contrib/blob/717f757d69c098e1baf786d3e7c03efacf2bbfaf/src/com/mebigfatguy/fbcontrib/detect/MoreDumbMethods.java
Thanks, Kevin! I totally forgot that these detectors landed in fb-contrib. I rebased my patch here, but I was having trouble getting the tests to run. With the detectors in fb-contrib, I don't think moving them to FindBugs master is worth the extra effort.
OK, thanks and sorry that it didn't made into FB.
Closing as out of date.
Regards,
Andrey