On Tue, Nov 20, 2001 at 03:43:52PM +0100, Anton Berezin wrote: > On Tue, Nov 20, 2001 at 04:27:03PM +0200, Ruslan Ermilov wrote: > > On Tue, Nov 20, 2001 at 03:12:50PM +0100, Anton Berezin wrote: > > > On Tue, Nov 20, 2001 at 03:02:39PM +0200, Ruslan Ermilov wrote: > > > > > While this is indeed a good thing to do, this is completely > > > > unrelated to the above mentioned problem, and should be done > > > > separately. Here's the list of src/ files that do not check the > > > > return value of initgroups(3), and may need to be fixed, but some > > > > of them explicitly ignore the result to indicate the fact they > > > > consider this error non-fatal. > > > > > libexec/ftpd/ftpd.c > > > > libexec/rexecd/rexecd.c > > > > usr.bin/calendar/calendar.c > > > > usr.sbin/inetd/inetd.c > > > > > > There used to be *many* more problematic files. Please see > > > > > > >http://www.freebsd.org/cgi/getmsg.cgi?fetch=801566+0+/usr/local/www/db/text/2001/freebsd-stable/20010722.freebsd-stable > > > > > > To my knowledge, only printjob.c was fixed, though I have not looked > > > into every file in the list since then. > > > Yes, but I specifically left contrib/ and crypto/ files, and files that > > do not check the result of other calls like setgrp() etc. > > We do not want to omit contrib/ files, since the whole hoopla started > because of the contrib/cvs/. > > > > But as I said in the private message, I do not feel strongly about > > > this, and I think that the fix can be safely committed. I do not > > > think these things are quite unrelated, though. :-) > > > Not checking the return value is always BAD except when (not) done > > intentionally (flagged by a(void)ing the return value of a function), > > whether or not a function in question prints some diagnostic output on > > standard error; that's why I still think these problems are in fact > > unrelated. :-) > > In this case your own version of the fix should be modified from > > + getgrouplist(uname, agroup, groups, &ngroups); > + return (setgroups(ngroups, groups); > > to > > + (void) getgrouplist(uname, agroup, groups, &ngroups); > + return (setgroups(ngroups, groups); > > , to be pedantic. :-) > Not actually, because getgrouplist(3) is special in that -1 is not actually an error, but an indication that the resulting array was too small to hold all groups. :-)
> The point I am trying to (not very strongly) make is that we at least > have some indication that there is a problem with the current behavior > (with the exception of the daemons with closed/redirected to /dev/null > stderr). By (rightfully) fixing initgroups(), we loose even this > precious little diagnostic we have. That's why initgroups() fix and the > code audit are probably best done at the same time - unless we can > guarantee the audit part will not be forgotten. > Sure, you can just change the synopsis of your PR. :-) Cheers, -- Ruslan Ermilov Oracle Developer/DBA, [EMAIL PROTECTED] Sunbay Software AG, [EMAIL PROTECTED] FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
