Earlier this week, I saw this code being recommended on Stack Overflow. The code contains a nasty, but rather subtle bug. The version I saw has been fixed now, but I thought there were some interesting lessons to learn by looking at the problems in some detail.

Let’s start by working out what the bug is. Here’s the code:

On first glance, it seems fine. It uses the common “open or die” idiom. It uses the modern approach of using a lexical filehandle. It even uses the three-argument version of “open()”. Code like has appeared in huge numbers of Perl programs for years. What can possibly be the problem?

I’ll give you a couple of minutes to have a closer look and work out what you think the problem is.

[ … time passes … ]

So what do you think? Do you see what the problem is?

The problem is that there is no error checking.

“What do you mean, Dave?” I hear you say. “There’s error checking there – I can see it plainly.” Some of you might even be wondering if I’m going senile.

And, yes, it certainly looks like it checks for errors. But the error checking doesn’t work. Let me prove that to you. We can check it with a simple command line program.

You would expect to see the “die” message there. But it doesn’t appear. Ok, perhaps I’m lying. Perhaps I really do have a file called “not.there”. Let’s try another, slightly different, version of the code.

And there we see the error message. That file really doesn’t exist.

So what went wrong with the first version? Of course, a good way to start working that out is to compare the two versions and look at the differences between them. The difference here is that when I put parentheses around the parameters to “open()” it started working. And when you fix things by adding parentheses it’s a pretty sure bet that the problem comes down to precedence.

The order of precedence for Perl operators is listed in perldoc perlop. If you look at that list you’ll see that the “or” operator we used (“||”) is at position 16 on the list. But what other operators are we using in our code? The answer is lurking down at position 21 on the list. When we call a Perl built-in function without using parentheses around the parameters, it’s known as a list operator. And list operators have rather low precedence.

All of which means that our original code is actually parsed as if we had written it like this:

Notice the parentheses that have appeared around $file and (crucially) the whole “or die” clause. That means that the bracketed expression is evaluated and passed to “open()” as its third argument. And when Perl evaluates that expression, it does that clever “Boolean short-circuiting” thing. An expression of “A || B” evaluates A first and if that is true, it returns it. Only if A is false will it go on to evaluate B and return that. In our case, the filename will always be true (well, unless you have a file called “0”) so the second half of the expression (the “or die…” bit) is never evaluated and, effectively, ignored.

Which is why I said, back at the start, that this code has no error checking at all – that’s literally true as the error checking has no effect at all.

So how do we fix it? Well, we’ve already seen one approach – you can explicitly add parentheses around the arguments to “open()”. But Perl programmers don’t like to use unnecessary punctuation and I’m sure I’ve seen this written without parentheses, so how does that work?

If you take another look at the table of operator precedence and look down below the list operators, you’ll see another “or” operator (the one that’s actually the word “or”, rather than punctuation). It’s right at the bottom of the list – at position 24. And that means we can use that version without needing the parentheses around the parameters to “open()”.

And that’s the version that you’ll see in most codebases. But, as we’ve seen, it’s vitally important to use the correct version of the “or” operator.

The worst thing about this bug is that it appears at the worst time. If your file exists and you can open it successfully, then everything works fine. Things only go wrong when… well, when things go wrong. If you can’t open your file for some reason, you won’t know about it. Which is bad.

So it’s important to test that your code works correctly when things go wrong. And that’s why we have modules like Test::Exception. You could write a test program like this:

And it would fail every time. But if you switched to the other “or” operator, it will work.

There’s one other approach you can take. You can use autodie in your code and just forget about adding “or die” to any of your calls to “open()”.

This is an easy bug to introduce into your code and a hard one to track down. Who’s confident that it doesn’t appear in any of their code?