Real bugs found in Sakai and related bug pattern

Owned by Former user (Deleted)
Last updated: Apr 01, 2008Version comment
5 min read

Introduction

Location of daily bug pattern reports against trunk (findbugs only):

Location of per project report including qj pro and PMD results and mentioning duplicate code http://qa1-nl.sakaiproject.org/codereview/trunk/

Bug Parade

Findbugs

Findbugs have currently 220 or so rules, below are a list of examples against rule type and if possible Jira

Rule: IS2_INCONSISTENT_SYNC (Inconsistent synchronization) SAK-13269
Thread1 calls doWork() and it obtains a lock on the object so no other thread can concurrently access code guarded by this lock. Thread2 tries to call reset() and it is allowed to proceed since it is not guarded by the lock held by Thread1. T1 has just checked that field b is not null. It will try to dereference b.toString() but in the meantime T2 has cleared b(by calling reset()) and T1 will throw a NullPointerException even though it has just tried to prevent just that from happening.

Possible solutions:

  • shrink the synchronized block and make a defensive copy of b.
  • synchronize all accesses of b too.

The book 'Java concurrency in practice' is a good starting point for a more thorough explanation.

public class Demo {
	private StringBuilder a = new StringBuilder();

	private StringBuilder b = new StringBuilder();

	public void reset() {
		b = null;
	}
	
	public synchronized String doWork() {
		if (b != null) {
			a.append(b.toString());
		}
		return a.toString();		
	}
}

Rule: IL_INFINITE_LOOP (Potential infinite loop)  SAK-12916 

207             int nr = fin.read(buffer, 0, bsize);
208             OutputStream out = res.getOutputStream();
209             while (nr > 0)
210             {
211                 out.write(buffer, 0, nr)
212             }

Rule: IL_INFINITE_RECURSIVE_LOOP

38      public boolean equal(Object o){
39          return this.equal(o);
40      }

Rule: ES_COMPARING_STRINGS_WITH_EQ SAK-12947

274             if ((_defaultCatalog != null) && (conn.getCatalog() != _defaultCatalog))
275             {
276                 conn.setCatalog(_defaultCatalog);
277             }

Rule: PMD PlaceLiteral http://jira.sakaiproject.org/jira/browse/SAK-13291
there are 928 places in Sakai with the following bug pattern:

if(sortColumn.equals("title")) {

if sortColumn is null then a nullpointer exception is generated. Defensive programming practice suggests that it should be
"title".equals then if sortColumn is null then false is returned.

Better still you could concentrate your strings at the top of the class to make the code easier to read.

See:
http://qa1-nl.sakaiproject.org/codereview/bug_dashboard/pmd_literals_first.html

Hint:
The TFTP project in Eclipse includes does static code review based on > 70 rules. After analysis, Eclipse can quick fix this bug pattern.
See: http://www.eclipse.org/projects/project_summary.php?projectid=tptp.platform

Rule: RV_RETURN_VALUE_IGNORED SAK-12979
value.replaceAll returns the replaced string, but does not replace the string in value itself.

517 public static String unEscapeHtml(String value)
518     {
519         if (value == null) return "";
520         if (value.equals("")) return "";
521         value.replaceAll("&lt;", "<");
522         value.replaceAll("&gt;", ">");
523         value.replaceAll("&amp;", "&");
524         value.replaceAll("&quot;", "\"");
525         return value
526     }

Rule: FE_TEST_IF_EQUAL_TO_NOT_A_NUMBER SAK-12944
Details:special semantics of NaN, no value is equal to Nan, including NaN.

1302            if (!(studentAnswerNum ==  Float.NaN || answer1Num ==  Float.NaN|| answer2Num ==  Float.NaN)){
1303            matchresult=((answer1Num <= studentAnswerNum) && (answer2Num >= studentAnswerNum))
1304            }

Rule: NP_ALWAYS_NULL SAK-12948
lines 8992,8996 both notAddedOfficialAccounts ,notAddedNonOfficialAccounts are null

8984        // update the not added user list
8985        String notAddedOfficialAccounts = null;
8986        String notAddedNonOfficialAccounts = null;
8987        for (Iterator iEIds = eIdRoles.keySet().iterator(); iEIds.hasNext();) {
8988            String iEId = (String) iEIds.next();
8989            if (!addedParticipantEIds.contains(iEId)) {
8990                if (iEId.indexOf(EMAIL_CHAR) == -1) {
8991                    // no email in eid
8992                    notAddedOfficialAccounts = notAddedOfficialAccounts
8993                            .concat(iEId + "\n");
8994                } else {
8995                    // email in eid
8996                    notAddedNonOfficialAccounts = notAddedNonOfficialAccounts
8997                            .concat(iEId + "\n");
8998                }
8999            }
9000        }

Rule: STCAL_STATIC_SIMPLE_DATE_FORMAT_INSTANCE  SAK-13068

As the JavaDoc states, DateFormats are inherently unsafe for multithreaded use. Sharing a single instance across thread boundaries without proper synchronization will result in erratic behavior of the application.

You may also experience serialization problems.
Using an instance field is recommended.

For more information on this see Sun Bug #6231579 and Sun Bug #6178997.
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6231579
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6178997

Rule:  SBSC_USE_STRINGBUFFER_CONCATENATION, WMI_WRONG_MAP_ITERATOR SAK-13039

Two patterns:
1. Inefficient iterations
2. String concatenation

Updated nightly - http://qa1-nl.sakaiproject.org/codereview/bug_dashboard/findbugs_generic_performance_it.html
See: http://javaantipatterns.wordpress.com/2007/11/22/accessing-the-map-values-using-keyset-iterator

1. Inefficient iterations

A common mistake is to retrieve values from a Map while iterating over the Map keys with keySet(). Calling Map.get(key) for each entry is expensive and should be avoided for better performance.

Use entrySet() iterator to avoid the Map.get(key) lookup.


//Bad:

for (Object key: map.keySet())
doSomething(map.get(key));

//Good:

for (Map.Entry entry: map.entrySet())
doSomething(entry.getValue());

 

2. Poor string concatenation - can lead to a cost quadratic in the number of iterations

(SBSC_USE_STRINGBUFFER_CONCATENATION)

The method seems to be building a String using concatenation in a loop. In each iteration, the String is converted to a StringBuffer/StringBuilder, appended to, and converted back to a String. This can lead to a cost quadratic in the number of iterations, as the growing string is recopied in each iteration.

Better performance can be obtained by using a StringBuffer (or StringBuilder in Java 1.5) explicitly.

For example:

// This is bad
  String s = "";
  for (int i = 0; i < field.length; ++i) {
    s = s + field[i];
  }

  // This is better
  StringBuffer buf = new StringBuffer();
  for (int i = 0; i < field.length; ++i) {
    buf.append(field[i]);
  }
  String s = buf.toString();