How's your C?

if ((options == (__WCLONE|__WALL)) && (current->uid = 0))
retval = -EINVAL;

Ya, i noticed the "= 0" right away. Programming in C/C++ for years makes you spot those pretty quickly. I wasn't really sure what the significance of any of these variables is, but it certainly seemed like an odd thing to do. What I did notice and the article didn't seem to mention is that this also prevents retval from ever being assigned -EINVAL because anthing && with 0 is 0.

One good practice to get into is to writing your conditons as follows:

if (0 == current->uid) ...

Accidently missing an equals sign here will attempt to assign a value to a literal constant which generates a compiler error. Great way to guard against such oopsies. But it does look weird and programmers who are not used to writing in languages that allow assignment inside conditionals think you're strange when they see you do that.
 
Glaucus said:
I wasn't really sure what the significance of any of these variables is, but it certainly seemed like an odd thing to do.
Click to expand...
current->uid = 0
Well, I'm not sure what the defined values refer to but in linux land, if the kernel switches a process's user id to 0 that means it's running as root. That means that any process that calls wait4 with the appropriate options escalates to root and can then do whatever the heck it wants. It could be a dumb programming error or it could be deliberate.

There are those that say you should never ascribe to malice that which can be explained by stupidity. That's probably not the position you should take if your concern is security.
 
Ya the article makes a good case that this is unlikely an error. It also explained the context so I understand the implications.

Back to coding styles, an even better way to do that kind of check would be to call a method that returns the uid. Or in this case, a method that indicates if the current user is root (boolean). That code is kinda sloppy if you ask me. And you can tell Linus I said so. ;)

Sent from my Nexus S using Tapatalk 2
 
Glaucus said:
Back to coding styles, an even better way to do that kind of check would be to call a method that returns the uid. Or in this case, a method that indicates if the current user is root (boolean). That code is kinda sloppy if you ask me. And you can tell Linus I said so. ;)
Click to expand...

Linus would probably say that the kernel is in C and that a memory fetch is cheaper than a call and cycles count in the kernel.
 
FluffyMcDeath said:
Linus would probably say that the kernel is in C and that a memory fetch is cheaper than a call and cycles count in the kernel.
Click to expand...
Ya, I thought of that after I posted it, but meh. I could argue for a #define macro to check for the user id. C can't provide proper data abstraction but there's no reason why you should litter your code with uid == 0 comparisons either. Anyway, my comment was more directed towards those amongst us who are still starting off with this stuff.
 
How on earth can a program make the user root so easily? I do not know the Linux kernal, but there's gotta be more checks before a program can do something so core-ish.
Even so, it can be a bug in a hot fix. Some rogue programmers don't look at their stuff as critically as others.
The '='-bug is something I always see immediately, yet sometimes I make this error, as a typo, when coding in a hurry. Besides, the IF statement can work without executing the '=' part,
if "(options == (__WCLONE|__WALL))" is untrue, the other part of the statement will never be executed, and as this part of the check may be the prime objective of the programmer, the lack of errors will not make the programmer looking with a critical eye at his code.
 
FluffyMcDeath said:
Linus would probably say that the kernel is in C and that a memory fetch is cheaper than a call and cycles count in the kernel.
Click to expand...
One could think of an efficient way to do it Glaucus' way. I kind-of-like the boolean part, but I'd prefer using an 8 bit flag register with a static (read-only) bitwise comparison function.
 
One of the comments says:
In addition, parentheses were not required for the final comparison. This was done to prevent compiler warnings. This looks deliberate.
Click to expand...


To my untrained eye this is also how it looks.
 
Robert said:
One of the comments says:


To my untrained eye this is also how it looks.
Click to expand...
Depends on your programming style. If I remember correctly, some compilers compile faster using such a parenthesis style.
And, personally I use parenthesis the same way, because when comparisons become more and more complex, it's a way to keep it readable/manageable.
 
You must log in or register to reply here.
Back
Top