Buggy Code

A Subtle Bug

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?


Also published on Medium.


Posted

in

by

Comments

11 responses to “A Subtle Bug”

  1. Mark Fowler Avatar

    PerlCritic would have caught that.

    1. Dave Cross Avatar

      Are you sure?

      $ cat open
      use strict;
      use warnings;
      
      open my $fh, '<', 'not.there' || die $!;
      $ perlcritic --brutal open
      Code not contained in explicit package at line 1, column 1.  Violates encapsulation.  (Severity: 4)
      No package-scoped "$VERSION" variable found at line 1, column 1.  See page 404 of PBP.  (Severity: 2)
      Close filehandles as soon as possible after opening them at line 4, column 1.  See page 209 of PBP.  (Severity: 4)
      Module does not end with "1;" at line 4, column 1.  Must end with a recognizable true value.  (Severity: 4)
      "die" used instead of "croak" at line 4, column 34.  See page 283 of PBP.  (Severity: 3)
      Magic punctuation variable $! used at line 4, column 38.  See page 79 of PBP.  (Severity: 2)
      
  2. Antred Avatar
    Antred

    I always use parentheses for argument lists and little understanding for why so many Perl coders insist on not doing so.

    1. Andrew DeFaria Avatar

      Really? Do you parenthesize Perl’s print or die statements. They are functions. I don’t and I don’t for functions I write ‘cept when required to disambiguate situations like this (that I always use the “or” word for anyway).

      1. Bill Costa Avatar
        Bill Costa

        As a matter of fact — I do. When I looked at the example, the first thing I wanted to do is add parentheses. Larry likes omitting parentheses because (I guess) it makes Perl more like a natural language. But I always use them because I don’t want to have to remember when you can leave them out and when you can’t, and the rules for precedence.

        I also think it makes the code *easier* not harder to read.

        But then again, I like Lisp.

  3. Jim Bacon (BOFTX) Avatar
    Jim Bacon (BOFTX)

    This bug is as old as Perl itself. I recall seeing it pointed out back in the very early 90s. It is a BIG reason why I almost always use parens for built-ins even though they are not needed just so the order of precedence is visible to less experienced coders.

  4. ysth Avatar

    The answer is not to add parentheses (though you certainly can), the answer is to always use the low precedence not, and, or, and xor for flow control.

  5. Jessica K McIntosh Avatar
    Jessica K McIntosh

    I use parenthesis most of the time just so I don’t have to think about situations like that. I got used to it when I learned Perl and was reinforced when I learned Lisp. It’s not that much more typing. I can also never remember operator precedence so I really have to.

  6. jonasbn Avatar

    Hi Dave, Thanks for a marvelous article. I have implemented a first shot at a policy to detect this bug available on GitHub. Feedback, pull-requests etc. most welcome check it out: https://github.com/jonasbn/perl-critic-policy-inputoutput-prohibitlogicaloperatorerrorhandling – BTW suggestions for a better name are most welcome ๐Ÿ™‚

  7. polettix Avatar

    … to say nothing about file “0”…

    1. Dave Cross Avatar

      I did mention that in the post.

Leave a Reply

This site uses Akismet to reduce spam. Learn how your comment data is processed.