On Mon, Oct 10, 2011 at 6:43 PM, Antoine Martin wrote: >> I have been using this patch for days without any visible side effects. >> Can I please get some reviewed-by / acks? >>
Any functional issues aside, your patch will need some style polishing before it can get any rubber stamps: > Subject: [PATCH xserver] check for elevated privileges not uid=0 (V3) First, the V3 in the subject should be inside the []'s to not be included in the commit message. Check other commits to see if there is a common practice of capitalization or module or section prefix in this line if you really want to please. +static Bool privsTested = FALSE; +static Bool privsElevated = FALSE; +Bool xf86PrivsElevated(void) You must have messed up from V2 to V3 because the variables ended outside the function again. Note that in V2 there was missing a blank line after the variable declarations. + uid_t ruid, euid, suid; + gid_t rgid, egid, sgid; + if ((getresuid(&ruid, &euid, &suid) == 0) && Missing blank line again. + */ + unsigned int oldeuid; + oldeuid = geteuid(); And again. + (getresgid(&rgid, &egid, &sgid) == 0)) + privsElevated = (euid != suid) || (egid != sgid); +#else + /* + * If there are saved ID's the process might still be priviledged privileged + * even though the above test succeeded. If issetugid() and I also consider double space after period a typographical mistake, but I am not gonna run a crusade here :) + if (seteuid(0) != 0) { + privsElevated = FALSE; You use != 0 to check for failure here... + } else { + if (seteuid(oldeuid) == -1) { ... but == -1 here. Is there any reason for not being consistent? + /* XXX ouch, coudn't get back to original uid + what can we do ??? */ If we get to this point, the code in the patch has failed. It should apologize deeply and offer your phone number for technical support :) Seriously, it should print an "internal error" message of some kind, maybe including __function__. Just quitting silently and leave a cryptic 127 that might or not propagate to the user does not cut it. + _exit(127); Why do you use _exit() and not exit() here? This is not e.g. a forked child that should escape normal clean-up? Best regards, Tormod _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel