Hi, On Fri, Mar 19, 2010 at 6:22 PM, Andrew Psaltis <ampsal...@gmail.com> wrote: > Hi again. > > After looking at the previous patch a little bit, it seems that there were > some bugs in handling authorization errors, but did not have time to correct > them until recently. This (new) patch should correct them. The > caveats/comments in the previous message still apply.
Sorry for not replying earlier but there are several problems here - you make the patch hard to review - real effort is needed to even take a quick glance at it. Use bugzilla (as the docs tell you to) - and never gzip patches. - patch is not in standard git patch format - which means real effort is needed to get it into the tree - there's white-space junk in the patch - the patch introduces compiler warnings. As you can see, the first two complaints are kinda the reason that I'm not responding until now - you just don't make it very easy to review. And since I've been busy working on other stuff, things that are hard to review goes in the end of the queue. More detailed comments follows below. Note that most of these things are minor and should be easy to fix - please follow up in bugzilla with a patch that addresses these comments... and we'll take it from there and get the patch in. Thanks! >- $(NULL) >+ polkitagenthelperprivate.c polkitagenthelperprivate.h You need to use trailing $(NULL) here. > +polkit_agent_helper_1_SOURCES += $(NULL) This isn't needed. > +extern char *crypt (); Really? You are throwing away type-safety here. Instead, do as crypt(3) man page is suggesting (by defining _XOPEN_SOURCE). Also, while compiling this code polkitagenthelper-shadow.c: In function âmainâ: polkitagenthelper-shadow.c:59: warning: implicit declaration of function âclearenvâ polkitagenthelper-shadow.c:59: warning: nested extern declaration of âclearenvâ polkitagenthelper-shadow.c:63: warning: implicit declaration of function âsetenvâ polkitagenthelper-shadow.c:63: warning: nested extern declaration of âsetenvâ polkitagenthelper-shadow.c: In function âshadow_authenticateâ: polkitagenthelper-shadow.c:180: warning: implicit declaration of function âusleepâ polkitagenthelper-shadow.c:180: warning: nested extern declaration of âusleepâ which is caused by missing includes. This probably also needs fixing when using the pam backend. > + if(!shadow_authenticate (shadow)) > +static int > +shadow_authenticate(struct spwd *shadow) You need to use a gboolean here instead of abusing the ! operator and the int type. The style is also wrong (need space between if and the starting parenthesis). > + /* Speak PAM to the daemon, thanks to David Zeuthen for the idea. */ Technically you are just speaking to libpolkit-agent-1 (or whoever launched the helper). No need for this comment. + /* Ask shadow about the user requesting authentication */ + if ((shadow = getspnam (user_to_auth)) == NULL) Split into separate lines. + fprintf(stderr, "polkit-agent-helper-1: could not get shadow information for%.100s", user_to_auth); Just use %s. + /* Check whether the user's password has expired */ + time(&tm); Lacks space between function name and starting parenthesis. Also tm is a bad name, use 'now' instead as that is a lot more... logical. Also, it's easier to read if you write it as now = time (NULL); + if( shadow->sp_max >= 0 && (shadow->sp_lstchg + shadow->sp_max) * 60 * 60 * 24 <= tm) Lacks space between if and starting parenthesis. > + if( shadow->sp_inact >= 0 && (shadow->sp_lstchg + shadow->sp_max + > shadow->sp_inact) * 60 * 60 * 24 <= tm) Ditto. Seriously, what's with all the whitespace? It doesn't inspire a lot of confidence to find random whitespace... and this is supposed to be security-sensitive code. > if (strcmp (shadow->sp_pwdp, crypt (passwd, shadow->sp_pwdp)) != 0) > goto error; This is really nasty. - crypt(3) may return NULL (e.g. ENOSYS, see the man page) - you make it look like crypt() has no side-effects while it actually changes passwd object So, split this into multiple lines and check return value of crypt(3). You also need to add a comment explaining why you use the salt that you use. > --- /dev/null > +++ b/src/polkitagent/polkitagenthelperprivate.c Lots of problems here. First of all, you need to include config.h otherwise HAVE_CLEARENV won't work. Second of all, you need to include stdlib.h for clearenv() in case HAVE_CLEARENV is set. Third, the compiler warns about this file, if you had payed any attention to warnings we wouldn't have these issues. > +#ifdef POLKIT_AUTHW_PAM Spelling error. Seriously, if you had tried building with pam (which you need to) and looked at compiler warnings it would say something about open_session() being defined but not used. David _______________________________________________ polkit-devel mailing list polkit-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/polkit-devel