I had a great time hanging out with my colleagues Bob and Amie yesterday at the HUB, talking with students, spotting defects and handing out yo-yos. Thanks to all who came out, and to the SWE for putting on a great event.
To follow up on the puzzle I posted yesterday: the terrible flaw, which most people spotted right away, was that the expression geteuid != 0 was of course intended to be geteuid() != 0. The code as written compares the function pointer to null, which it never is, and therefore the right side is always true, and therefore the conditional falls into the “fail with an error” branch more often than it ought to. The program succeeds if the user really is root, or if they are “sudo” root. It is intended to succeed also if the user is “effectively” root, but it does not. Thank goodness in this case the program fails to a secure mode! It is not at all difficult to imagine a situation where such an accidental function pointer usage causes the program to fail into the insecure mode. In any event, Coverity’s checker catches this one. (And of course more modern languages like C# do not allow you to use methods in a context other than a call or delegate conversion.)
There are of course any number of other flaws in this fragment. First, it’s now considered bad form to check for root like this; rather, check to see if the user is granted an appropriate permission. Second, the code is hard to read if you do not know the convention that the root user gets magical id zero by default; the code could be much more self-documenting. And so on; several people made good observations in the comments.
Next time on FAIC: You can build a hollow house out of solid bricks, and you can build a deadlocking program out of threadsafe methods too.
Fabulous adventures in coding 
In all fairness, you didn’t mention the language. I had assumed C# and I doubt I’m the only one. Perhaps it was obvious to you that it wasn’t C#, but I don’t have any experience with any other language that could compile that code snippet. 🙂
‘Modern’ isn’t the criterium here. Compiling this code with gcc gives the following message:
warning: the comparison will always evaluate as ‘true’ for the address of ‘geteuid’ will never be NULL [-Waddress]
So you don’t even need coverity, as long as you make an effort to reduce the number of warnings to zero.
There are many modern languages for which it is impractical to do the kind of static analysis that widely-used, freely available C compilers have built in.
@Dave Well it was pretty obviously not idiomatic c# code and I assume Eric assumed pretty much everybody here has written c code in the past and would recognize the obvious strcmp.
@michiel To be fair, the only reason those c compilers have this myriad of warnings is exactly because all those “modern” languages wouldn’t even allow most of these things (e.g. why do static analysis for use of uninitialized variables, when the language already explicitly makes that an error?). If you ignore these, C compilers aren’t noteworthy for finding more higher level problems.
Coverity yo-yos? How do I get one of these?
There’s a career fair coming up at the University of Calgary next week, and I can find out if my colleague Tyler is going to be handing out yo-yos. Toronto is just down the highway from Calgary, right? 🙂
@voo: The fact that issues like the aforementioned code will not prevent compilers from producing an executable could be regarded as either an annoyance or a feature; what would perhaps be most helpful would be an easy means of indicating that something should be regarded as a constant for code-generation purposes, but not for semantic-analysis-based warnings (conditional tests that evaluate to a constant with one set of build options may do meaningful with another). Another thing that would be helpful from a language-design perspective would be to increase the “Hamming distance” between expressions that have very different meanings. Obviously C is what is is, but a future language could require that anything that was directly invokable as a function must either have arguments attached or apply some other symbol to take the address, rather than applying some particular meaning to the bare identifier.
@voo, when you say ‘modern’, you’re probably thinking about C#. The majority of code produced isn’t made in C# nowadays, or even Java. It’s written in JavaScript, Ruby, Python, PHP. Languages where _assigning_ 0 to geteuid doesn’t even cause the interpreter to blink, and forget about it caring about comparison.
Yes, it’s probably time to retire C for everything but embedded and kernel programming. Yes, it’s probably a good idea to burn most books about C++ in existence, excepting a few museum copies to warn future generations.
But in a time where all acquired knowledge about making software manageable for human brains is happily thrown overboard, it’s very silly to see those two languages as the main causes of bugs.