From: Henry Ford <henryfordkjv@gmail.com>
Subject: Re: Fix less(1) crash on invalid tags file
To: Omar Polo <op@omarpolo.com>
Cc: tech@openbsd.org
Date: Mon, 16 Mar 2026 13:02:41 -0400

Thanks for taking the time to review this,

On Sun, Mar 15, 2026 at 15:16:39 PM -0400, 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.

Wow, that was pretty stupid. Obviously you are correct, thanks.
Next time I will take more care instead of trying to blindly adapt a patch
for a different codebase.

> 
> (haven't seen whether that causes crashes as well)

Currently less(1) will go the beginning of the file for any tag identified
by a negative line number, as if the line number were 1. In practice I think
it is reasonble to skip such tags as I doubt anyone is actually using them.
Neither ctags(1) nor mandoc(1) from base, nor universal-ctags, exuberant ctags,
hasktags, or vtags from ports will generate such a tag. They all use one of the
standard /^[pattern]$/, ?[pattern]?, or [line number] formats.

Your diff makes sense to me, fwiw.

> 
> 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.