friendly ping

"Omar Polo" <op@omarpolo.com> wrote:
> Hello,
> 
> Henry Ford <henryfordkjv@gmail.com> wrote:
> > If less(1) encounters a tags file which includes a tag identified by a line
> > number of 0, then less(1) will crash if the user attempts to jump to this tag.
> > 
> > You can reproduce this with the following commands:
> > echo > hi
> > echo Evil hi 0 > hi.tags
> > less -T hi.tags -t Evil
> > 
> > This will result in a segmentation fault from less(1).
> > 
> > This happens because less(1) internally uses a line number of 0 to denote that
> > a tag is identified by a pattern instead of a line number. This is reasonable
> > to do since line numbers in tag identifiers are indexed from 1 and not 0. If
> > the line number for a tag actually was 0 then less(1) will get confused and
> > attempt to access the pattern string for that tag, which is NULL since it is
> > identified by a line number.
> > 
> > To fix this I have made less(1) ignore any tag identified by a line number of 0,
> > as is done for other invalid entries in a tags file. This is consistent with
> > vi(1), which treats such tags as invalid and complains if the user tries to jump
> > to them. mg(1) does not support using line numbers as tag identifiers and will
> > instead treat them as patterns.
> 
> (TIL that the pattern could be a number as well)
> 
> > This was fixed in https://github.com/gwsw/less/pull/743 (opened by me) with
> > effectively the same patch as below. With the patch applied the commands
> > which previously caused less to crash will instead result in a message
> > telling the user that the specified tag could not be found.
> > 
> > --- usr.bin/less/tags.c
> > +++ usr.bin/less/tags.c
> > @@ -282,6 +282,9 @@ findctag(char *tag)
> >  				p--;
> >  			*p = '\0';
> >  		}
> > +		if (taglinenum == 0)
> > +			/* Line numbers start from 1. */
> > +			continue;
> 
> at this point we might have entered the if (err) case, and taglinenum
> might be have set to zero explicitly.  Also, note that getnum() parses
> happily also negative numbers, even if they don't cause a crash.
> 
> (haven't seen whether that causes crashes as well)
> 
> I'd propose the below diff instead.
> 
> >  		tp = maketagent(tagfile, taglinenum, tagpattern, tagendline);
> >  		TAG_INS(tp);
> >  		total++;

diff /usr/src
path + /usr/src
commit - 6965f4adac1ec8b9b9fcb3516ed56fae271049dc
blob - ddc8d6541df92d7f9cdfb28b9d70813857fc4ad8
file + usr.bin/less/tags.c
--- usr.bin/less/tags.c
+++ usr.bin/less/tags.c
@@ -260,6 +260,10 @@ findctag(char *tag)
 		 */
 		tagendline = 0;
 		taglinenum = getnum(&p, 0, &err);
+		if (!err && taglinenum <= 0) {
+			/* line numbers must be positive */
+			continue;
+		}
 		if (err) {
 			/*
 			 * No, it must be a pattern.