Re: misc/15421 (was: Re: initgroups)
On Mon, Nov 19, 2001 at 10:28:55PM +0600, Max Khon wrote:
hi, there!
On Mon, Nov 19, 2001 at 06:19:50PM +0200, Ruslan Ermilov wrote:
Can setgroups return a positive number? If so, you've just changed
the semantics of the funtion; before, it used to return 0 on 0 or a
positive number.
No. setgroups() is a syscall, and as such returns either 0 or -1.
Also, is removing the _warn() really the only thing you want to
accomplish? It should probably be seperate.
I have intended to commit the below patch for almost a year now,
just haven't had enough time to actually fo it. NetBSD runs with
this fix since 1999.
Index: initgroups.c
===
RCS file: /home/ncvs/src/lib/libc/gen/initgroups.c,v
retrieving revision 1.4
diff -u -p -r1.4 initgroups.c
--- initgroups.c2001/08/29 13:52:26 1.4
+++ initgroups.c2001/11/19 16:16:11
@@ -56,12 +56,6 @@ initgroups(uname, agroup)
int groups[NGROUPS], ngroups;
ngroups = NGROUPS;
- if (getgrouplist(uname, agroup, groups, ngroups) 0)
- warnx(%s is in too many groups, using first %d,
- uname, ngroups);
- if (setgroups(ngroups, groups) 0) {
- _warn(setgroups);
- return (-1);
- }
- return (0);
+ getgrouplist(uname, agroup, groups, ngroups);
+ return (setgroups(ngroups, groups);
There's a missing closing parenthesis above, sorry.
Index: initgroups.3
[...]
I asked tobez (he is an originator and he took responsibility on this PR)
and he said that src/ must be audited also -- he said that some initgroups()
callers do not print error message because initgroups() did this
previously.
I'll try to do this before this weekend and I will post combined patch
to audit@
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
Cheers,
--
Ruslan Ermilov Oracle Developer/DBA,
[EMAIL PROTECTED] Sunbay Software AG,
[EMAIL PROTECTED] FreeBSD committer,
+380.652.512.251Simferopol, 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
Re: misc/15421 (was: Re: initgroups)
On Tue, Nov 20, 2001 at 03:02:39PM +0200, Ruslan Ermilov wrote: On Mon, Nov 19, 2001 at 10:28:55PM +0600, Max Khon wrote: I asked tobez (he is an originator and he took responsibility on this PR) and he said that src/ must be audited also -- he said that some initgroups() callers do not print error message because initgroups() did this previously. I'll try to do this before this weekend and I will post combined patch to audit@ 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. 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. :-) Cheers, \Anton. -- | Anton Berezin| FreeBSD: The power to serve | | catpipe Systems ApS _ _ |_ | http://www.FreeBSD.org | | [EMAIL PROTECTED](_(_|| |[EMAIL PROTECTED] | | +45 7021 0050| Private: [EMAIL PROTECTED] | To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: misc/15421 (was: Re: initgroups)
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: On Mon, Nov 19, 2001 at 10:28:55PM +0600, Max Khon wrote: I asked tobez (he is an originator and he took responsibility on this PR) and he said that src/ must be audited also -- he said that some initgroups() callers do not print error message because initgroups() did this previously. I'll try to do this before this weekend and I will post combined patch to audit@ 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. 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. :-) Cheers, -- Ruslan Ermilov Oracle Developer/DBA, [EMAIL PROTECTED] Sunbay Software AG, [EMAIL PROTECTED] FreeBSD committer, +380.652.512.251Simferopol, 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
Re: misc/15421 (was: Re: initgroups)
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. :-) 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. Cheers, $Anton. -- | Anton Berezin| FreeBSD: The power to serve | | catpipe Systems ApS _ _ |_ | http://www.FreeBSD.org | | [EMAIL PROTECTED](_(_|| |[EMAIL PROTECTED] | | +45 7021 0050| Private: [EMAIL PROTECTED] | To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: misc/15421 (was: Re: initgroups)
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.251Simferopol, 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
Re: misc/15421 (was: Re: initgroups)
On Tue, Nov 13, 2001 at 02:28:57PM -0800, Terry Lambert wrote:
Max Khon wrote:
hi, there!
Any objections if I will commit the following patch (see PR/15421)?
Can setgroups return a positive number? If so, you've just changed
the semantics of the funtion; before, it used to return 0 on 0 or a
positive number.
No. setgroups() is a syscall, and as such returns either 0 or -1.
Also, is removing the _warn() really the only thing you want to
accomplish? It should probably be seperate.
I have intended to commit the below patch for almost a year now,
just haven't had enough time to actually fo it. NetBSD runs with
this fix since 1999.
Index: initgroups.c
===
RCS file: /home/ncvs/src/lib/libc/gen/initgroups.c,v
retrieving revision 1.4
diff -u -p -r1.4 initgroups.c
--- initgroups.c2001/08/29 13:52:26 1.4
+++ initgroups.c2001/11/19 16:16:11
@@ -56,12 +56,6 @@ initgroups(uname, agroup)
int groups[NGROUPS], ngroups;
ngroups = NGROUPS;
- if (getgrouplist(uname, agroup, groups, ngroups) 0)
- warnx(%s is in too many groups, using first %d,
- uname, ngroups);
- if (setgroups(ngroups, groups) 0) {
- _warn(setgroups);
- return (-1);
- }
- return (0);
+ getgrouplist(uname, agroup, groups, ngroups);
+ return (setgroups(ngroups, groups);
}
Index: initgroups.3
===
RCS file: /home/ncvs/src/lib/libc/gen/initgroups.3,v
retrieving revision 1.10
diff -u -p -r1.10 initgroups.3
--- initgroups.32001/10/01 16:08:51 1.10
+++ initgroups.32001/11/19 16:16:11
@@ -61,10 +61,14 @@ is automatically included in the groups
Typically this value is given as
the group number from the password file.
.Sh RETURN VALUES
+.Rv -std initgroups
+.Sh ERRORS
The
.Fn initgroups
-function
-returns \-1 if it was not invoked by the super-user.
+function may fail and set
+.Va errno
+for any of the errors specified for the library function
+.Xr setgroups 2 .
.Sh SEE ALSO
.Xr setgroups 2 ,
.Xr getgrouplist 3
Cheers,
--
Ruslan Ermilov Oracle Developer/DBA,
[EMAIL PROTECTED] Sunbay Software AG,
[EMAIL PROTECTED] FreeBSD committer,
+380.652.512.251Simferopol, 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
Re: misc/15421 (was: Re: initgroups)
hi, there!
On Mon, Nov 19, 2001 at 06:19:50PM +0200, Ruslan Ermilov wrote:
Can setgroups return a positive number? If so, you've just changed
the semantics of the funtion; before, it used to return 0 on 0 or a
positive number.
No. setgroups() is a syscall, and as such returns either 0 or -1.
Also, is removing the _warn() really the only thing you want to
accomplish? It should probably be seperate.
I have intended to commit the below patch for almost a year now,
just haven't had enough time to actually fo it. NetBSD runs with
this fix since 1999.
Index: initgroups.c
===
RCS file: /home/ncvs/src/lib/libc/gen/initgroups.c,v
retrieving revision 1.4
diff -u -p -r1.4 initgroups.c
--- initgroups.c 2001/08/29 13:52:26 1.4
+++ initgroups.c 2001/11/19 16:16:11
@@ -56,12 +56,6 @@ initgroups(uname, agroup)
int groups[NGROUPS], ngroups;
ngroups = NGROUPS;
- if (getgrouplist(uname, agroup, groups, ngroups) 0)
- warnx(%s is in too many groups, using first %d,
- uname, ngroups);
- if (setgroups(ngroups, groups) 0) {
- _warn(setgroups);
- return (-1);
- }
- return (0);
+ getgrouplist(uname, agroup, groups, ngroups);
+ return (setgroups(ngroups, groups);
}
Index: initgroups.3
===
RCS file: /home/ncvs/src/lib/libc/gen/initgroups.3,v
retrieving revision 1.10
diff -u -p -r1.10 initgroups.3
--- initgroups.3 2001/10/01 16:08:51 1.10
+++ initgroups.3 2001/11/19 16:16:11
@@ -61,10 +61,14 @@ is automatically included in the groups
Typically this value is given as
the group number from the password file.
.Sh RETURN VALUES
+.Rv -std initgroups
+.Sh ERRORS
The
.Fn initgroups
-function
-returns \-1 if it was not invoked by the super-user.
+function may fail and set
+.Va errno
+for any of the errors specified for the library function
+.Xr setgroups 2 .
.Sh SEE ALSO
.Xr setgroups 2 ,
.Xr getgrouplist 3
ok
I asked tobez (he is an originator and he took responsibility on this PR)
and he said that src/ must be audited also -- he said that some initgroups()
callers do not print error message because initgroups() did this
previously.
I'll try to do this before this weekend and I will post combined patch
to audit@
/fjoe
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
RE: initgroups
On 13-Nov-01 Max Khon wrote:
hi, there!
Any objections if I will commit the following patch (see PR/15421)?
Index: initgroups.c
===
RCS file: /home/ncvs/src/lib/libc/gen/initgroups.c,v
retrieving revision 1.4
diff -u -r1.4 initgroups.c
--- initgroups.c 2001/08/29 13:52:26 1.4
+++ initgroups.c 2001/11/13 20:17:03
@@ -59,9 +59,5 @@
if (getgrouplist(uname, agroup, groups, ngroups) 0)
warnx(%s is in too many groups, using first %d,
uname, ngroups);
- if (setgroups(ngroups, groups) 0) {
- _warn(setgroups);
- return (-1);
- }
- return (0);
+ return setgroups(ngroups, groups);
}
Style nit:
return (setgruops(ngroups, groups));
Also, commit the manpage patch in the PR as well.
--
John Baldwin [EMAIL PROTECTED]http://www.FreeBSD.org/~jhb/
Power Users Use the Power to Serve! - http://www.FreeBSD.org/
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: initgroups
On Wed, Nov 14, 2001 at 02:19:56AM +0600, Max Khon wrote: Any objections if I will commit the following patch (see PR/15421)? Does the man page need a note about setting errno? David. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: initgroups
Max Khon wrote: hi, there! Any objections if I will commit the following patch (see PR/15421)? Can setgroups return a positive number? If so, you've just changed the semantics of the funtion; before, it used to return 0 on 0 or a positive number. Also, is removing the _warn() really the only thing you want to accomplish? It should probably be seperate. -- Terry To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: initgroups
John Baldwin wrote: + return setgroups(ngroups, groups); } Style nit: return (setgruops(ngroups, groups)); return (setgroups(ngroups, groups)); (avoiding cut-and-paste error). -- Terry To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: initgroups
On 13-Nov-01 Terry Lambert wrote: John Baldwin wrote: + return setgroups(ngroups, groups); } Style nit: return (setgruops(ngroups, groups)); return (setgroups(ngroups, groups)); (avoiding cut-and-paste error). Yes, my mailer eats tabs and sucks as is well documented, the point was the ()'s. :) -- John Baldwin [EMAIL PROTECTED]http://www.FreeBSD.org/~jhb/ Power Users Use the Power to Serve! - http://www.FreeBSD.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: initgroups
John Baldwin wrote: return (setgruops(ngroups, groups)); return (setgroups(ngroups, groups)); (avoiding cut-and-paste error). Yes, my mailer eats tabs and sucks as is well documented, the point was the ()'s. :) I didn't fix the tabs; I fixed the typo; I lined it up to make it more obvious this time... :-). -- Terry To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: initgroups
On 13-Nov-01 Terry Lambert wrote: John Baldwin wrote: return (setgruops(ngroups, groups)); return (setgroups(ngroups, groups)); (avoiding cut-and-paste error). Yes, my mailer eats tabs and sucks as is well documented, the point was the ()'s. :) I didn't fix the tabs; I fixed the typo; I lined it up to make it more obvious this time... :-). Oh geez. :-P I've always said that compilers make good spell checkers. :) -- John Baldwin [EMAIL PROTECTED]http://www.FreeBSD.org/~jhb/ Power Users Use the Power to Serve! - http://www.FreeBSD.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
