From: "Omar Polo" <op@omarpolo.com>
Subject: Re: Fix less(1) crash on invalid tags file
To: Henry Ford <henryfordkjv@gmail.com>
Cc: tech@openbsd.org
Date: Sun, 15 Mar 2026 20:16:39 +0100

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.