Bug 31265

Hi there,

After looking at PR docs/31265 I would like to submit this patch for
possible review.  The patch originally provided in this PR is (I
believe) incorrect in stating that usernames must be followed by a
newline character.  Possibly this was corrected in FreeBSD sometime
after the PR was submitted.  I cannot duplicate this behaviour in
FreeBSD 4.4-STABLE.

I have written the following patch for the man page, and hopefully
someone can look over it and maybe commit it for me if they feel it's
ok?

Thanks,
Marc

-- 
I've learned that being kind is more important than being right.
        -- Andy Rooney

Marc Silver <marcs@draenor.org> writes:

> I have written the following patch for the man page, and hopefully
> someone can look over it and maybe commit it for me if they feel it's
> ok?

The re-write needs work.  I haven't taken the time to re-do the tests
upon which my patch was based, but looking at this code from 4.5-Pre
leads me to believe nothing's changed relative to the newline issue.

    /usr/src/usr.sbin/cron/lib/misc.c

	while (fgets(line, MAX_TEMPSTR, file)) {
		if (line[0] != '\0')
			line[strlen(line)-1] = '\0';
		if (0 == strcmp(line, string))
			return TRUE;

Note that it wipes out the last character before the null, which is
the newline character except for a line which does not end with a 
newline, so that a user name at the end of the file with no trailing
newline will be ignored.  It still looks to me like a user name which
ends the file will be ignored.  I suspect a testing error.

As for the rest, if you don't like my terse precision, then please add
some more verbiage to yours to indicate that the user names must also
not have any characters (such as whitespace) following them on the line.

I think it's rather silly to say "you must use the correct format".

I think "username" is not and should not be one word, despite the fact
that it often is.

Saying "Only one username must be added per line." doesn't read well.
The sentence which follows that doesn't need a comma; the "or" clause is
too short to require it.

Saying "Comments may also be inserted into this file." leaves one asking
what a legal comment is.  My patch provided that information.  A comment
is a line which contains something other than a user name followed by a
newline.

On Tue, Jan 01, 2002 at 09:11:40PM -0800, Gary W. Swearingen wrote:
> The re-write needs work.  I haven't taken the time to re-do the tests
> upon which my patch was based, but looking at this code from 4.5-Pre
> leads me to believe nothing's changed relative to the newline issue.
> 
>     /usr/src/usr.sbin/cron/lib/misc.c
> 
> 	while (fgets(line, MAX_TEMPSTR, file)) {
> 		if (line[0] != '\0')
> 			line[strlen(line)-1] = '\0';
> 		if (0 == strcmp(line, string))
> 			return TRUE;
> 
> Note that it wipes out the last character before the null, which is
> the newline character except for a line which does not end with a 
> newline, so that a user name at the end of the file with no trailing
> newline will be ignored.  It still looks to me like a user name which
> ends the file will be ignored.  I suspect a testing error.

Firstly, let me say that my c knowledge is not overly impressive, and
while I understand more or less what's going on there, I'm not about to
get into a discussion re: it.

I have however re-tested this, and on my machine (4.4-STABLE compiled on
18 Dec 2001) a username at the end of a file _without_ a newline
character is still allowed/denied correctly as it's supposed to be.
There's no testing error here... it works just fine for me.

> As for the rest, if you don't like my terse precision, then please add
> some more verbiage to yours to indicate that the user names must also
> not have any characters (such as whitespace) following them on the
> line.

It's not about liking or disliking your 'terse precision'.  The reason I
resubmitted this was because my unpatched crontab.1 file seemed
different to yours, and there had been no action on this PR.  I'm merely
trying to revive it and hopefully get something done about the problem
you found.

> I think it's rather silly to say "you must use the correct format".

You're entitled to your opinion.  Do you have a better suggestion?

> I think "username" is not and should not be one word, despite the fact
> that it often is.

Surely this is a 'religious' debate?  It has nothing to do with whether
or not the patch is relevant.  I personally do believe that 'username'
is correct.  A quick search of the man pages in the source tree found
the word 'username' 139 times... so I'm fairly sure that it's widely
accepted.

> Saying "Only one username must be added per line." doesn't read well.
> The sentence which follows that doesn't need a comma; the "or" clause
> is too short to require it.

Thanks for the grammatical heads-up.  I'll fix that too...

> Saying "Comments may also be inserted into this file." leaves one
> asking what a legal comment is.  My patch provided that information.
> A comment is a line which contains something other than a user name
> followed by a newline.

This has already been brought up, and I'll submit a change later today
to explain what a comment is.

Quite frankly, I couldn't care if your patch or mine is used at the end
of the day.  As long as the man page gets corrected.

- Marc

Btw.  Your reply seems somewhat agressive... if I've offended you by
submitting this, then I apologise.

Marc Silver <marcs@draenor.org> writes:

>  On Tue, Jan 01, 2002 at 09:11:40PM -0800, Gary W. Swearingen wrote:
>  > [stuff at this level]

>  There's no testing error here... it works just fine for me.

OK, so I re-tested too and find I still have the error.  Shall we try to
determine whether I've got old code or you've got a testing error or...?

I find the newest file under /usr/src/usr.sbin/cron to be
/usr/src/usr.sbin/cron/crontab/crontab.c dated Jun 15  2001; size 13933;
md5 3d7a88f82812c75c7a1ab0d3d806dc86

My /usr/bin/crontab is size 24872, but I'm not sure if that SHOULD match
yours.

I cvsup'd "RELENG_4/src-all" 26'dec'01 and did "make world" stuff then.

Running as "jojo", I get this with "jojo" at the end of "/var/cron/allow".

$ crontab -l
crontab: you (jojo) are not allowed to use this program

$ hd /var/cron/allow | tail -2
00000370  20 72 6f 6f 74 2e 0a 72  6f 6f 74 0a 6a 6f 6a 6f  | root..root.jojo|
00000380

>  .. my unpatched crontab.1 file seemed different to yours, ...

My /usr/src/usr.sbin/cron/crontab/crontab.1 and my 4.3R one both have
  3282 Dec  8  2000 /usr/src/usr.sbin/cron/crontab/crontab.1

>  > I think it's rather silly to say "you must use the correct format".
>  
>  You're entitled to your opinion.  Do you have a better suggestion?

Omit the sentence which contains it.  It should go without saying.

>  Btw.  Your reply seems somewhat agressive... if I've offended you by
>  submitting this, then I apologise.

I despise aggressiveness and usually restrain myself, but I occasionally
repay inferred offense with implied offense.  This time I tried to
merely state what I thought of your changes and not what I thought of your
seeming carelessness in making them.  I spent a long time carefully
wording my patch.  I would have reacted better to criticism and
suggestions for improvement than to a flat-out replacement, especially
when I find it to be both poorly expressed and inaccurate.  Maybe this
is a lesson to push one's PR on people after it gets ignored.

You were kind enough to ask for my thoughts on a rewrite.  Here they
are.  Please don't take offense.

+In order to use the
+.Pa allow
+and
+.Pa deny
+files the following format must be adhered to:
+.Pp
+.Bl -tag -width indent
+.It Fl 
+Only one username may be added per line.

Verbose.  And the last line sounds strange.  Sorry I can't explain it
well; something strange about adding to a line which doesn't exist.
How about replacing all with: "In these files, each user name must be
on a separate line with no other characters on the line.  Other lines
are ignored and may be used for comments.  A bug in the program causes
a user name at the end of the file to be ignored; end the file with a
newline character to avoid the bug."  (The last part depending on what
our research shows.)

+.It Fl 
+The username should not have any characters (such as whitespace) 
+preceding it or it will be ignored.  

Preceding AND FOLLOWING IT ON THE SAME LINE.  (Depending on research.)

+.It Fl 
+The username must start at the beginning of a line.  (^)

Redundant.  The previous sentence has the same meaning.

+.It Fl
+Anything else will be ignored as a comment.

Anything else than what?  Will a "#..." after the user name be ignored
as a comment?   Yes, but so will the user name.

+.El
+.Bl -tag -width indent
+.Pp
+If neither of these files exists, then depending on site-dependent 
+configuration parameters, only the super user will be allowed to use 
+this command, or all users will be able to use this command.

I had left that for another PR, but that leaves one asking:  What
configuration parameters?

Please send me date/size/md5 info as I did above or any other ideas
you might have on determining whether I have old software or why we
get different testing results.  (I also checked misc.c and crontab.c in
http://www.freebsd.org/cgi/cvsweb.cgi/src/usr.sbin/cron/
and it seems I've got current code for RELENG_4.)

Oh, I tested by simply editing "allow" and running "crontab -l" as a
normal user.

Hi there,

On Wed, Jan 02, 2002 at 03:17:27PM -0800, Gary W. Swearingen wrote:
> I find the newest file under /usr/src/usr.sbin/cron to be
> /usr/src/usr.sbin/cron/crontab/crontab.c dated Jun 15  2001; size 13933;
> md5 3d7a88f82812c75c7a1ab0d3d806dc86

My MD5 for that file is 3d7a88f82812c75c7a1ab0d3d806dc86, which matches
yours...

> My /usr/bin/crontab is size 24872, but I'm not sure if that SHOULD
> match yours.

My file size matches yours.  :)

> I cvsup'd "RELENG_4/src-all" 26'dec'01 and did "make world" stuff then.

I cvsup'd RELENG_4/src-all on 18 December 2001 and did make world then.
I have since then cvsup'd every two-three days...

> Running as "jojo", I get this with "jojo" at the end of "/var/cron/allow".
> 
> $ crontab -l
> crontab: you (jojo) are not allowed to use this program
> 
> $ hd /var/cron/allow | tail -2
> 00000370  20 72 6f 6f 74 2e 0a 72  6f 6f 74 0a 6a 6f 6a 6f  | root..root.jojo|
> 00000380

Running as "marcs", I get this with the username at the end of
/var/cron/allow...

pressure (1) :  crontab -l
0       10      *       *       * ( /usr/local/www/generate.pl )
<snip>

pressure (2) :  hd /var/cron/allow | tail -2
00000000  72 6f 6f 74 0a 6d 61 72  63 73 0a |root.marcs.| 0000000b

Here I see my output is slightly different to yours, though I am not
sure why...

[09:20:38] ~
pressure (3) :  cat /var/cron/allow 
root
marcs
[09:20:41] ~
pressure (4) :

Note that I dont have a newline character after the last username.

> My /usr/src/usr.sbin/cron/crontab/crontab.1 and my 4.3R one both have
>   3282 Dec  8  2000 /usr/src/usr.sbin/cron/crontab/crontab.1

My file sizes/dates match yours.

> I despise aggressiveness and usually restrain myself, but I
> occasionally repay inferred offense with implied offense.  This time I
> tried to merely state what I thought of your changes and not what I
> thought of your seeming carelessness in making them.  I spent a long
> time carefully wording my patch.  I would have reacted better to
> criticism and suggestions for improvement than to a flat-out
> replacement, especially when I find it to be both poorly expressed and
> inaccurate.  Maybe this is a lesson to push one's PR on people after
> it gets ignored.

You were mistaken in thinking that I implied offense in the first place.
I never said your patch was 'poorly expressed' nor 'inaccurate', though
I see you feel the need to criticise mine and be quite harsh in your
replies.  In fact, you show little or no 'restraint' as far as I'm
concerned.

I will not waste my time further with this debate.  It's obvious that
you have resent me sending in this patch, so I will 'withdraw' my patch
and my work on this PR can be ignored.  

> +If neither of these files exists, then depending on site-dependent 
> +configuration parameters, only the super user will be allowed to use 
> +this command, or all users will be able to use this command.
> 
> I had left that for another PR, but that leaves one asking:  What
> configuration parameters?

This portion was not written by me, but was merely re-ordered... 

I wish you the best of luck in getting your patch committed and hope to
have better dealings with you in the future.

- Marc

-- 
I've learned that being kind is more important than being right.
        -- Andy Rooney

Marc Silver <marcs@draenor.org> writes:

> On Wed, Jan 02, 2002 at 03:17:27PM -0800, Gary W. Swearingen wrote:
> > [stuff at this level]
> > 
> > $ hd /var/cron/allow | tail -2
> > 00000370  20 72 6f 6f 74 2e 0a 72  6f 6f 74 0a 6a 6f 6a 6f  | root..root.jojo|
> > 00000380
> 
> pressure (2) :  hd /var/cron/allow | tail -2
> 00000000  72 6f 6f 74 0a 6d 61 72  63 73 0a |root.marcs.| 0000000b
> 
> Here I see my output is slightly different to yours, though I am not
> sure why...

I consider this to be conclusive evidence that your testing was
erroneous so that your only explicit criticism of the original patch
should be ignored.  You apparently have not tested with a user name at
the end of the file without a trailing newline character as you claimed.

I still think that user names must each start at the beginning of a line
and must be followed immediately by a newline character -- all other
lines are ignored.  (They probably also must be in some user account
config file(s) but that should go without saying.)

As for the state of the PR, I'm still not aware of anything particularly
wrong with the original patch and hope that committers will consider
that patch.  I'll consider modifying it in response to criticisms of it
and suggestions for its improvement, of course.

In case you're still interested, that "0a" just after "73" in your "hd"
output is the "newline" character at issue.  Your file ends with a
newline character (so your user name is used); mine doesn't (so my user
name is ignored).

As you can see, newline at the end of a file is a troublesome thing.
Some programs consider newline to be a part of a line and insist on
ending the file with a line with a newline.  My editor (XEmacs) will not
move the cursor below the "jojo" line of my file, but will move it below
the "marcs" line of yours and will increment the line count in doing so.
The "wc" program would show one more line in yours than mine.  The
"diff" program seems to see a blank line in both files and shows two
blank lines as being different.  Your shell (and my pdksh) seem to
ignore the empty line so it doesn't show as a blank line.

(I'll go to private mail for some social debugging.)

> I've learned that being kind is more important than being right.
>        -- Andy Rooney

I've been listening to Andy for at least 25 years and even read one of
his books.  He's the kind of guy that has likely also said the exact
opposite because it's just as profound or cute or whatever he's going
for at the time.  (In fact, "kind" and "right" are more important than
the other in different circumstances.)  He's made a career out of
complaining about what people do wrong on a show that does the same and
they all have found it important to be unkind and, worse, unfair.

Just to keep it alive...

On Thu, Jan 03, 2002 at 03:40:03PM -0800 I heard the voice of
Gary W. Swearingen, and lo! it spake thus:
>  
>  I still think that user names must each start at the beginning of a line
>  and must be followed immediately by a newline character -- all other
>  lines are ignored.  (They probably also must be in some user account
>  config file(s) but that should go without saying.)
>  
>  As for the state of the PR, I'm still not aware of anything particularly
>  wrong with the original patch and hope that committers will consider
>  that patch.  I'll consider modifying it in response to criticisms of it
>  and suggestions for its improvement, of course.

Yeesh, you people always take the hard way   ;p
The Right Solution (IMO, of course) is to fix cron to not make what is a
rather non-intuitive choice in parsing the files, then document the
now-simple one-username-per-line.  Something like (untested):


Index: crontab/crontab.1
===================================================================
RCS file: /usr/cvs/src/usr.sbin/cron/crontab/crontab.1,v
retrieving revision 1.9
diff -u -r1.9 crontab.1
--- crontab.1	2000/11/20 20:09:43	1.9
+++ crontab.1	2002/03/11 22:12:53
@@ -58,7 +58,9 @@
 file in order to use this command.  If neither of these files exists, then
 depending on site-dependent configuration parameters, only the super user
 will be allowed to use this command, or all users will be able to use this
-command.
+command.  The format of these files is one username per line, with no
+leading or trailing whitespace.  Lines with other information on them
+will be ignored, and so can be used for comments.
 .Pp
 The first form of this command is used to install a new crontab from some
 named file or standard input if the pseudo-filename ``-'' is given.
Index: lib/misc.c
===================================================================
RCS file: /usr/cvs/src/usr.sbin/cron/lib/misc.c,v
retrieving revision 1.9
diff -u -r1.9 misc.c
--- misc.c	2000/05/23 13:44:00	1.9
+++ misc.c	2002/03/11 22:08:14
@@ -392,7 +392,8 @@
 	rewind(file);
 	while (fgets(line, MAX_TEMPSTR, file)) {
 		if (line[0] != '\0')
-			line[strlen(line)-1] = '\0';
+			if (line[strlen(line)-1] == '\n')
+				line[strlen(line)-1] = '\0';
 		if (0 == strcmp(line, string))
 			return TRUE;
 	}



>  blank lines as being different.  Your shell (and my pdksh) seem to
>  ignore the empty line so it doesn't show as a blank line.

Well, it's not an empty line that would show as a blank line; that would
require TWO \n's.  vis:
[16:09:51] mortis:~/tmp
(ttyp3):{931}% echo hi > out
[16:10:19] mortis:~/tmp
(ttyp3):{932}% echo -n hi > out2
[16:10:22] mortis:~/tmp
(ttyp3):{933}% cat out
hi
[16:10:23] mortis:~/tmp
(ttyp3):{934}% cat out2
hi[16:10:24] mortis:~/tmp
(ttyp3):{935}% 



-- 
Matthew Fuller     (MF4839)     |    fullermd@over-yonder.net
Unix Systems Administrator      |    fullermd@futuresouth.com
Specializing in FreeBSD         |    http://www.over-yonder.net/

"The only reason I'm burning my candle at both ends, is because I
      haven't figured out how to light the middle yet"

"Matthew D. Fuller" <fullermd@over-yonder.net> writes:

> The Right Solution (IMO, of course) is to fix cron to not make what is a
> rather non-intuitive choice in parsing the files, then document the
> now-simple one-username-per-line.  Something like (untested):

Looks OK to me, except that if I were doing it (which I'm not) and I
could trust the adduser(8) manual about alphanumerics & "_" only (which I
can't -- see adding_user(8)), then I'd allow for trailing comments by
adding this statement below yours:    line[strcspn(line, "#")] = '\0';

> >  Your shell (and my pdksh) seem to
> >  ignore the empty line so it doesn't show as a blank line.
> 
> Well, it's not an empty line that would show as a blank line; that would
> require TWO \n's.  vis:

It would show as a blank line in SOME software. The more intuitive
behavior would be to consider a newline character to start a new
line, even if empty or at the end of the file.  XEmacs, for instance,
shows it and counts it, considering it to form an empty line at the end
of the file.  But the shells and "wc" have good reasons for doing it the
other way, no doubt.

Comment 9 dwmalone 2002-03-17 14:03:27 UTC

Responsible Changed
From-To: freebsd-doc->dwmalone

I've committed a patch to -current and will merge it in a few weeks.

Comment 10 dwmalone 2002-04-28 23:45:59 UTC

State Changed
From-To: open->closed

Now merged to -stable. Thanks for the patch.