Errors in Public Libraries
A random anecdote about other people's code.
December 03, 2014We had a stack trace recurring with high frequency on our management hosts today. Digging into the issue, we found this (cleaned up) trace:
Internal Server Error
java.lang.NullPointerException:null
...OMElementImpl.removeAttribute(OMElementImpl.java:617)
...FOMElement.setAttributeValue(FOMElement.java:269)
...FOMElement.setAttributeValue(FOMElement.java:370)
I followed the stack to the Axiom library. This is a basic case of a public class not checking input. The NPE happens when we getQName()
(comments mine)
public void removeAttribute(OMAttribute attr) {
// attributes is a field of the class.
if (attributes != null) { // only null check.
// A second null check (of attr) should be here.
// First NPE site, since attr wasn't checked.
((OMAttributeImpl)attr).owner = null;
// This would NPE, too:
attributes.remove(attr.getQName());
}
}
In our case attr
was null. Pretty simple. There are some things we could do to protect ourselves here:
- Fix the Gross Conceptual Error of confusing
attributes
, a field, with attr, the thing you should probably be null checking. - Make it so the worst that method could do is nothing. Fixing the previous would obviate this case.
I Jumped up the stack to part of Abdera, also an Apache project. The following method is guaranteed to send a null to removeAttribute, explained inline. (warning: some sarcasm)
public <T extends Element> T setAttributeValue(QName qname, String value) {
OMAttribute attr = this.getAttribute(qname);
// attr may be empty or null.
// Does qname have a value?
if (attr != null && value != null) {
// This is the safest code in the method.
attr.setAttributeValue(value);
} else {
if (value != null) {
// Don't know if qname is null, using it anyway!
String uri = qname.getNamespaceURI();
// If we get past this line qname qname isn't null.
String prefix = qname.getPrefix();
if (uri != null) {
// uri isn't null, prefix is unchecked! (not sure if bad?)
OMNamespace ns = findNamespace(uri, prefix);
// WARNING: bare if.
if (ns == null)
ns = factory.createOMNamespace(uri, prefix);
// was null prefix ok above?
attr = factory.createOMAttribute(
qname.getLocalPart(), ns, value);
} else {
attr = factory.createOMAttribute(
qname.getLocalPart(), null, value);
}
// attr has a value iff factory worked.
// WARNING: bare if.
if (attr != null)
addAttribute(attr);
} else {
// Value is definitely null, attr is still unchecked.
removeAttribute(attr); // In the stacktrace
}
}
return (T)this;
}
There are multiple contributing factors:
- Potential issues with null values as input to a public method.
(attr != null && value != null) != (attr == null && value == null)
, like the code implies in the else case. (The && becomes || due to DeMorgan's Laws)- Second bare if makes it look like the else case belongs to it.
- removeAttribute is from OMElement, which FOMElement inherits. The unfortunate choice in the super-class exposes a bug in this code.
I think this is a great example of checking dependencies, code reviews, and doing logic naïvely by hand (in comments) to be certain of your results.