The University of Washington chapter of the Society of Women Engineers is putting on their quarterly career fair today at the Husky Union Building, and I’ll be there with a couple of my fabulous Coverity colleagues. If you happen to be a UW student reading this and want to stop on by for a round of my favourite game, “Spot the Defect” please do. We’ll be presenting a whole pile of buggy code; some of the defects will be quite straightforward and some of them will be subtle, but they all have a lesson.
If you want to play along at home, here’s one of the easy ones; this code was in a real used-by-customers product when Coverity’s static analysis engine discovered it:
{
...
if (!strcmp(argv[i], "-configure"))
{
if (getuid() != 0 && geteuid != 0)
{
puts("Only root can use -configure.n");
exit(1);
}
}
xf86DoConfigure = TRUE;
xf86AllowMouseOpenFail = TRUE;
return 1;
}
Can you spot the defect and describe its consequence?
Anyways, like I said, if you’re a UW student then stop on by the booth and we’ll chat. The fair is open 12:30 to 5:30; if you’re a SWE member then you get early admission and can come in any time after 12:00. Hope to see you there!
Next time on FAIC: The solution to today’s puzzle.
Fabulous adventures in coding 
Obviously, it should be “geteuid() != 0”.
“geteuid != 0” compares the address of geteuid function with NULL, which is true… I’d say always, but who knows?
Therefore, the condition is equivalent to “if (getuid() != 0)”.
Not knowing unix programming, I am not sure whether it is a bug or a feature 🙂
The most apparent result will be that “sudo buggytool -configure” won’t work. Not the kind of “feature” I’d enjoy running across 😉
The problem is not with sudo or su, they change the real uid, so the buggy code sees itself as launched by root and works.
The problem occurs when you change the owner of the program to root and set the setUID permission to the file: when a normal user executes it, his real uid is greater than 0 (he is not root) while his effective uid is 0 (root gives him the permission through the setUID). The program should work, but the check fails because geteuid != 0 is always true.
Well, if the condition is always false, it is almost definitely a feature 🙂
As far as I have read, explicit testing for rootness is discouraged, because the non-root user may have the required privileges.
So the behaviour of the “buggy” program is: if the user doesn’t have necessary rights, the program crashes. If the user does have them, the program works.
“Correct” program would behave like this: if the user is root, the program works. If the user is not root, but has the required privileges, the program fails; the user is frustrated. If the user doesn’t have necessary rights, the program exits.
Now it is clear that the “correct” behaviour is worse than “buggy” one, except for the lack of an error message 🙂
I’m mostly joking, because it’s obvious that the real program might be more complex. But still, isn’t similar logic the reason for the “3.95” version number of Win95?
I think the “correct” behaviour is not to check for rootness all, but to check for the appropriate permissions instead. Eg, if you need to write to /etc/buggytool.conf then try opening that file for writing and see if it succeeds. If not, then you need to barf an error. User frustration doesn’t factor in to it 🙂
missing () on the second getuid?
When I read the code, I assumed that the effective user id had been cached in a variable called geteuid for some reason, and like xf86DoConfigure and friends, we merely couldn’t see the definition.
That is, it stands out and is clear and obvious, but not obviously wrong without knowing the rest of the code.
The missing parentheses after geteuid causes the function pointer to be compared against 0, the function will not be called (which was most likely the original intention). Also assuming gueteuid is not weak symbol.
Well, it’s not a bug, but I’d still slap a programmer for using ‘if(!strcmp(argv[i], “-configure”))’ over `if(strcmp(argv[i], “-configure”) == 0) `
`!strcmp` has the “NOT” operator in it, implying “argv[1] is NOT equal to “-configure”.
`strcmp()==0` has the EQUALS operator in it, implying argv[1] is EQUAL to “-configure”
So, to save barely two keystrokes, you get code that easy to mis-read.
My thoughts exactly. Why do people do that, I don’t know. Relict from the past? Want to make themselves look more “hackerish”?
It’s common enough to be a recognizable idiom. Doesn’t mean it’s a good idiom, but idiom nonetheless.
FWIW, shouldn’t the ‘xf86DoConfigure = TRUE’ bits also be inside the “if” block?
Given that we don’t know the definition of geteuid, I’d say the defect is that xf86DoConfigure is set to TRUE even if the -configure option is not set.
The question is how a static analysis tool identified this defect. Maybe due to an if condition containing only another if?
Does it strike anyone that errors, state of the art static analysis tools bring up on languages like C or C++ kinda impossible with better designed languages?
Can it be that there’s not as much sense for searching bad practices in code as for designing languages that make those bad practices impossible in a first place?
How many modern languages would allow a leg being shot this particular way anyway?
My thoughts exactly: I’m not a unix/linux person and it’s been years since I last did anything professional in C++, but that’s what I noticed: the function seems to work the same way when the argument (argv[i]) is set to “-configure” and the user is root AND if the argument is NOT set to configure, and the user is anything. I’m not sure if this is a defect or the desired behaviour, but is seems strange to me.
Assuming that code is from what I think it is from, the -configure option uses generic direct hardware access for purposes of hardware detection and writes a configuration file. This can only be done by the root user. Once that a configuration file is written, and perhaps modified, hardware detection is no longer necessary, and it becomes possible to configure the system so that it can be used by a non-root user. Regardless, it is definitely intentional: the logic matches the error message “Only root can use -configure”.
Oh, wait! I see what you mean now. Yes, the xf86DoConfigure = TRUE; and following statements belongs inside the if block. Was this perhaps a mistake in copying the code?
Is the bug that there’s no return statement immediately after exit(1)?
I noticed that as well, though I think the missing parenthesis after the geteuid function identifier is the more likely answer.
exit() terminates the program, so any following return statement would never be reached.
But what if exit() fails?!
“… you can build a deadlocking program out of threadsafe methods …”
This is obvious. A thread safe method could be “unsafe” in conjuction with another. Typical cases are shared locks such as lock(“someReusedString”), lock(this) and lock(typeof(CurrentClass)).
I assure you that it is inobvious to all the people who ask me whether they can “guarantee that a method is threadsafe” by applying some simple test.
Is this a bug in the “commenting system” of this blog? How come a comment about thread-safe methods/programs (a topic of another post) is under this puzzle post?
> whether they can “guarantee that a method is threadsafe” by applying some simple test.
Sure you can. For example: “The content of the method consists entirely of { return; }” is such a simple test. It, of course, has many false negatives. It’s probably possible to cast the net a _bit_ wider than that, though not as wide as the people who ask you for such a test would hope.
It seems strange that the function would set xf86DoConfigure = TRUE no matter what, even though it checked for the “-configure” flag. I think that that line should be moved up before the closing brace of the first “if” statement.
using strcmp at all is a security risk. You have to make sure that the string is properly 0-terminated, or use newer string libs or at least strncmp.
Using strcmp() to compare two arbitrary strings may be a security risk, but if one of the strings is a hard-coded literal, would not that not limit the number of bytes strcmp() could try to process? I’d be more worried about the behavior of strncmp() if the literal string were replaced with something longer but the value passed for length was not adjusted.
Eric, The tab characters in the code snippet seem to have broken the RSS feed.
Thanks for the note; I’ve fixed it.
As a fellow UWer, I am deeply disappointed in your use of UW as an abbreviation for University of Washington. Everyone knows that UW should only ever be used to refer to the University of Waterloo :).
I agree with @Mike’s comment, as well as this discussion on SO: http://stackoverflow.com/questions/4159910/check-if-user-is-root-in-c
I’d like to add however that the correct check should be just if (geteuid() != 0): otherwise the root user sudoing into a non-root account in order to execute suspicious code will fail.
The sad thing is that it took a static analyzer to discover this. If the code had been compiled with more warnings enabled (or possibly with more warnings fixed so that the relevant ones aren’t lost in the clutter), the compiler would probably have warned about the tautological “function != null” issue.
Way cool! Some very valid points! I appreciate you writing this article plus the rest of the site is really good.
Pingback: Integer division that rounds up | Fabulous adventures in coding