Comment by kaffeinecoma | Hacker News Reader

Comment by kaffeinecoma | Hacker News Reader

I once caused a serious, halt-the-enterprise production bug by "fixing" a problem found by FindBugs. This was Java code, something along the lines of:

  Boolean b = new Boolean(true);

The static analyzer correctly identified this as an unnecessary new object creation (style guides and good sense recommend you simply use Boolean.TRUE). I "fixed" it, and went on my way.

Little did I realize that this variable was actually a lock, and there was a synchronized(b) block later (and much deeper) in the code, which I effectively eliminated by removing the new.

In my defense I feel that the real bug here was that of documentation- had the variable been named something like "lock" I'd have understood immediately what was going on. But that doesn't make you feel much better when your team's been up all night fixing your bug!

Moral of the story: your codebase (especially if it's an older one) might actually be depending on its "bugs" for proper behavior. Think (and test) hard before applying suggested changes from static analysis.

The choice of a Boolean as a monitor object is a little odd. My understanding is that the usual convention is to create such objects via "new Object()", which is a little more obvious- the only reason you would ever call the base Object constructor is to produce something that can be used as a monitor.

It was a couple of years ago and I no longer have access to the codebase, but the business logic of that piece of code called for a boolean.

The problem was they they also (ab)used that field for the lock. It either should have been a separate field (of type Object, as you suggested) or use the existing Boolean, but call it "lock" or "monitor" or somesuch.

Sure, but the bug in this case is that there wasn't a comment specifying the reason for the unconventional behaviour.

I agree. If you know you're writing something that is un-idiomatic or you think its intended purpose will be a surprise to most readers, put a comment in explaining why.

    // we need a heap object so we can synchronize on it later
    Boolean b = new Boolean(true);

I'd also make the name a bit more descriptive.

This points to a deeper problem of static analysis, though - any analysis package without the ability to annotate code is _doomed_. The false positives will be so annoying that people will give up on it.

And for the people who work on SA systems - please give me a way to annotate that is not exclusively via comments. Especially once people use multiple SA packages, that is rather annoying :)

Don't forget the OpenSSL "bug" (using uninitialized memory) that was really on purpose, resulting in a critical flaw once changed.

In that instance too, lack of commenting to explain the behavior was also at fault.

Sure, it wasn't great code, but this also sounds like a flaw in the static analyzer. It should have been able to tell that the variable was being synchronized on, and recognized that using Boolean.TRUE would have been an unsafe change.

That's a weird line of code to begin with. Not only the naming was wrong, but the type was wrong too (Boolean instead of Object).

Could you check all the references where that b variable was used prior to changing the code?

There likely to be just one or two such places.

Sure I could have found all of the references easily. That's one of the reasons I still prefer Java over a dynamic language- my IDE can tell me instantly (option key + F7) where a particular object is used in the entire codebase. I just wasn't careful enough, because hey, what could possibly go wrong- it's a stupid boolean, right?

When I see unusual error in code - not only I check related code, but I may run svn blame. There are multiple benefits for it:

1) I can learn something new and what I thought was an error would turn out to be an interesting new coding trick.

2) If it was an error - not only we would fix it, but would also learn not to make such error in the future.

In particular, that person who made that error would learn to avoid it in the future.

Reply on news.ycombinator.com