The FindBugs Blog

Monday, October 15, 2007

Thread unsafe lazy initialization

I spent some time improving the detector for thread unsafe lazy initialization.

The following code is from JDK1.6.0, sun.security.provider.certpath.KeyChecker:


private static Set supportedExts;
public Set getSupportedExtensions() {
if (supportedExts == null) {
supportedExts = new HashSet();
supportedExts.add(PKIXExtensions.KeyUsage_Id.toString());
supportedExts.add(PKIXExtensions.ExtendedKeyUsage_Id.toString());
supportedExts.add(PKIXExtensions.SubjectAlternativeName_Id.toString());
supportedExts = Collections.unmodifiableSet(supportedExts);
}
return supportedExts;
}


So if two threads called getSupportedExtensions on two different KeyCheckers, not only could one thread see an incompletely initialized set, but one of the threads could also be handled a mutable reference to the set of supported extensions, which it could then use to add or remove supported extensions.

Oops...

Bill

Restarting the blog...

OK, yeah, I got swamped and haven't touched the blog in months.

I'll try to update it more regularly.

Bill

Monday, May 14, 2007

A closed source WTF: the longest second

OK, since I've pointed out a number of times that open source code contains a number of embarrassing bugs, I want to just emphasize that I've seen embarrassing code in every code base I've examined, including some propriatary closed source code bases.

I just want to share the following code, found in the code base of a highly respected tech company.

private static final String SECOND = "second";

public static String getTimeDelayStr(long delay) {

double roundDelay = 0;

// seconds
if (delay == 1)
return String.valueOf(new Double(Math.ceil(delay)).intValue()) + " " + SECOND;
....

Tuesday, May 01, 2007

Round and Round we go; Netbeans WTF

I found the following code in a recent snapshot of netbeans. If anyone has a clue as to what the developer was thinking, let me know. The code is in org.openide.awt.HtmlLabelUI:


private static int textWidth(String text, Graphics g, Font f, boolean html) {
if (text != null) {
if (html) {
return Math.round(
Math.round(
Math.ceil(
HtmlRenderer.renderHTML(
text, g, 0, 0, Integer.MAX_VALUE, Integer.MAX_VALUE, f, Color.BLACK,
HtmlRenderer.STYLE_CLIP, false
)
)
)
);
} else {
return Math.round(
Math.round(
Math.ceil(
HtmlRenderer.renderPlainString(
text, g, 0, 0, Integer.MAX_VALUE, Integer.MAX_VALUE, f, Color.BLACK,
HtmlRenderer.STYLE_CLIP, false
)
)
)
);
}
} else {
return 0;
}
}

Labels:

Sorry for the long absence, I'm back

I would up getting completely swamped with some other stuff at work and not updating the findbugs blog for some time. I hope to be posting more regularly here now.

Monday, November 13, 2006

WTF in javax.sql.rowset.serial.SerialRef

Sometimes, a bug report will point you to some code that just elicits a WTF response.

FindBugs generated a null pointer warning in javax.sql.rowset.serial.SerialRef, complaining about the call if (!object.equals(null)):

/**
* Returns an Object representing the SQL structured type
* to which this SerialRef object refers. The attributes
* of the structured type are mapped according to the given type map.
*
* @param map a java.util.Map object containing zero or
* more entries, with each entry consisting of 1) a String
* giving the fully qualified name of a UDT and 2) the
* Class object for the SQLData implementation
* that defines how the UDT is to be mapped
* @return an object instance resolved from the Ref reference and mapped
* according to the supplied type map
* @throws SerialException if an error is encountered in the reference
* resolution
*/
public Object getObject(java.util.Map<String,Class<?>> map)
throws SerialException
{
map = new Hashtable(map);
if (!object.equals(null)) {
return map.get(object);
} else {
throw new SerialException("The object is not set");
}
}


But when you look at that code, that seems to be the least of the problems.
Why is the map being converted into a new Hashtable, and what is this method intended to do?

Since the return type of this isn't Class, I'm guessing that it should
return something other than Class objects. I'm guessing it should create an instance of the class found in the Map. Does anyone know?

This is one of the places where using a generic type would have been very useful in documenting the specification of the method, even if it wasn't that useful to users of the method. If the method had been declared as:
public <T> T getObject(java.util.Map<String,Class<? extends T>> map)
then the relationship between the map and the return value would have been exceptionally clear. Of course, I don't know if this is the intended specification of the class.

P.S. Thanks to Sun's open sourcing of Java, I no longer have to worry about posting examples with source code from Sun's JDK.

Monday, September 25, 2006

A 10 year old null pointer bug

I was doing some historical analysis of FindBugs warnings in the JDK, and found that there is one null pointer warning that FindBugs generates on the 1.0.2 version of Sun's JDK that still exists in JDK 1.6.0-b99.
The bug is that if you call setHelpMenu(null) on a java.awt.MenuBar, and there is already an existing help menu, you'll get a null pointer exception. The buggy code is:

/**
* Sets the specified menu to be this menu bar's help menu.
* If this menu bar has an existing help menu, the old help menu is
* removed from the menu bar, and replaced with the specified menu.
* @param m the menu to be set as the help menu
*/
public void setHelpMenu(Menu m) {
synchronized (getTreeLock()) {
if (helpMenu == m) {
return;
}
if (helpMenu != null) {
remove(helpMenu);
}
if (m.parent != this) {
add(m);
}
helpMenu = m;
if (m != null) {
m.isHelpMenu = true;
m.parent = this;
MenuBarPeer peer = (MenuBarPeer)this.peer;
if (peer != null) {
if (m.peer == null) {
m.addNotify();
}
peer.addHelpMenu(m);
}
}
}
}

Now, the JavaDoc doesn't say that you should call setHelpMenu(null) to remove a help menu. But there is no other way to remove a help menu, and the check for if (m != null) suggests that the developer anticipated m being null. But we know that m can't be null here, because if m were null, a null pointer exception would have occurred at if (m.parent != this)

Why hasn't this been noticed or fixed in the past 10 years? I don't know, but I've now filed a bug report.

Monday, September 18, 2006

How do you fix an obvious bug?

Consider the following code (from Eclipse 3.2, org.eclipse.jdk.internal.core.NamedMember):

for (int i = 0; i < length; i++) {
String typeArgument = typeArguments[i];
typeArgument.replace('/', '.');
buffer.append(Signature.toString(typeArgument));
if (i < length-1)
buffer.append(',');
}

FindBugs points out that the return value of replace('/', '.') is being ignored and thus the call has no effect. It is very tempting to just "fix" the code to get

for (int i = 0; i < length; i++) {
String typeArgument = typeArguments[i];
typeArgument = typeArgument.replace('/', '.');
buffer.append(Signature.toString(typeArgument));
if (i < length-1)
buffer.append(',');
}

But does this actually fix the code? We probably don't have any test cases where the type argument contains a '/', or else we would have already have fixed the code. Or perhaps the buffer should contain slashes rather than dots, and the fact that the return value from replace is ignored is what makes the code correct. Or perhaps '.' would be the correct answer, but other code in the system depends upon the error in this code.

Static analysis tools, such as FindBugs, don't actually know what your code is supposed to do. Instead, they typically just find inconsistencies in your code. Invoking a String replace method and ignoring the return value is weird or inconsistent, but we don't actually know that using the return value is more correct.

So no easy answers. On recommendation is that when you see an obvious bug, maybe you shouldn't just bang out a quick fix. Instead, figure out why tests aren't showing a problem with this code, try to document the intended behavior and write a test case showing the existing code gives incorrect behavior.

Then, go make the obvious fix.


Bill Pugh

Saturday, September 16, 2006

Is Math.abs broken?

You'd think that Math.abs would return a non-negative integer. You'd be wrong. In fact, Math.abs(Integer.MIN_VALUE) = Integer.MIN_VALUE. The problem is that Integer.MIN_VALUE is -2147483648, and +2147483648 cannot be represented as a 32-bit signed integer (Integer.MAX_VALUE = 2147483647).
How can this bite you? It is not uncommon to see code such as

Object x = bucket[ Math.abs(y.hashCode()) % bucket.length]

Why is the call to Math.abs there? Because essentially all processors implement integer division as rounding towards zero, rather than towards negative infinity (as most mathmeticians and computer scientists would prefer). Java defines integer division the same way. As a result, a remainder computation can give a negative result if the dividend is negative (e.g., (-3)%2 == -1).

If we had just written
Object x = bucket[ y.hashCode() % bucket.length]
then roughly half the time, the remainder would be negative and we'd get an array index out of bound exception. So people will call Math.abs to obtain a non-negative dividend.
But as we've discussed, Math.abs doesn't guarantee a non-negative dividend. if the hashCode is Integer.MIN_VALUE, then the result of the Math.abs will be negative and we will likely get a negative remainder and in array index out of bounds exception. The same pattern/problem comes up when using random 32 bit signed integers rather than hashCodes.

I'm not sure whether to be relieved or worried that this problem would occur only about one time out of 4 billion. An error that occurs half the time is a lot easier to find via testing. An error that comes up once out of 4 billion will be almost impossible to reproduce or find through testing.

FindBugs 1.1 searches for this bug pattern. How often do we find it?

  • 7 times in eclipse-SDK-3.2-solaris-gtk
  • 6 times in jboss-4.0.2
  • 6 times in eclipse-SDK-3.2-solaris-gtk
  • twice in tomcat-4.1.31
  • 9 times in BEA Weblogic 9.0
  • 10 times in IBM WebSphere 6.0.3

The (quick) recommended way to fix this simply use a integer bitwise and to clear the high bit, ensuring that the result is non-negative.

Object x = bucket[ (y.hashCode() & Integer.MAX_VALUE) % bucket.length]

If you know that the length of the array is a power of two, you can also use:
Object x = bucket[ y.hashCode() & ( bucket.length - 1) ]
One quick word of warning: you might not want to assume that the hashCode function gives uniformly distributed values. Sun's HashMap using the following method to stir the bits of a hashCode to try to ensure a more uniformly distributed hash value:

/**
* Returns a hash value for the specified object. In addition to
* the object's own hashCode, this method applies a "supplemental
* hash function," which defends against poor quality hash functions.
* This is critical because HashMap uses power-of two length
* hash tables.


*
* The shift distances in this function were chosen as the result
* of an automated search over the entire four-dimensional search space.
*/
static int hash(Object x) {
int h = x.hashCode();
h += ~(h << 9);
h ^= (h >>> 14);
h += (h << 4);
h ^= (h >>> 10);
return h;
}


So, back to my original question: Is Math.abs broken? I'm not sure how it could be any different. There isn't any value that could be returned for Math.abs(Integer.MIN_VALUE) that would be correct. And making Math.abs(Integer.MIN_VALUE) throw exception wouldn't really improve the situation. So I wouldn't say it is broken; just dangerous.

Bill Pugh

Welcome to the FindBugs blog.

We're going to use this blog to post items of interest about FindBugs, static analysis tools, and other topics related to software quality.