Re: mandoc: fix a buffer over-read

Hello Josiah,

Josiah Frentsos wrote on Thu, Jul 30, 2026 at 06:29:57PM -0400:

> To reproduce:
> 	printf '\\B \\B' | MALLOC_OPTIONS=SU mandoc

Thank you for the clear report and for sending a patch!

> Index: roff_escape.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mandoc/roff_escape.c,v
> diff -p -u -r1.15 roff_escape.c
> --- roff_escape.c	16 May 2024 21:21:08 -0000	1.15
> +++ roff_escape.c	30 Jul 2026 22:12:41 -0000
> @@ -297,12 +297,13 @@ roff_escape(const char *buf, const int l
>  			if (rval != ESCAPE_EXPAND)
>  				rval = ESCAPE_ERROR;
>  			if (buf[inam] != 'D') {
> -				iendarg = iend = iarg + 1;
> +				if (buf[iarg] != '\0')
> +					iendarg = iend = iarg + 1;
>  				goto out;
>  			}
>  		}
>  		if (term == '\b')
> -			term = buf[iarg++];
> +			term = buf[iarg] == '\0' ? '\0' : buf[iarg++];
>  	} else if (term == '\0' && maxl == INT_MAX) {
>  		if (buf[inam] == 'n' && (buf[iarg] == '+' || buf[iarg] == '-'))
>  			iarg++;

Your patch looks correct to me from code inspection, but i committed
the following instead, which i believe is easier to read and verify,
and which also provides more rigorous error reporting:


Log Message:
-----------
When an escape sequence that requires an argument occurs at the end of an
input line and the argument is missing, abort parsing the line and report
an "incomplete escape sequence" error.

This fixes a read buffer overrun that Josiah Frentsos <[email protected]>
sent a different patch for.  Instead of always entering the argument
parsing code and detecting that there is no argument at two places in
the middle of that code, as Josiah proposed, i chose to instead check
up front that there is anything that can be parsed to begin with.
Also reminded by deraadt@, thanks!

Modified Files:
--------------
    mandoc:
        roff_escape.c

Revision Data
-------------
Index: roff_escape.c
===================================================================
RCS file: /home/cvs/mandoc/mandoc/roff_escape.c,v
diff -Lroff_escape.c -Lroff_escape.c -u -p -r1.15 -r1.16
--- roff_escape.c
+++ roff_escape.c
@@ -264,6 +264,13 @@ roff_escape(const char *buf, const int l
 		iendarg = iend = iarg;
 	}
 
+	/* Mandatory argument is missing. */
+
+	if (buf[iarg] == '\0' && (term != '\0' || maxl != INT_MAX)) {
+		err = MANDOCERR_ESC_INCOMPLETE;
+		goto out;
+	}
+
 	/* Decide how to end the argument. */
 
 	escterm = 0;

plus:

CVSROOT:	/cvs
Module name:	src
Changes by:	[email protected]	2026/08/08 08:21:31

Modified files:
	regress/usr.bin/mandoc/roff/esc: Makefile 
Added files:
	regress/usr.bin/mandoc/roff/esc: argmiss.in argmiss.out_ascii 
	                                 argmiss.out_lint 

Log message:
test handling of missing escape sequence arguments at EOL,
related to roff_escape.c rev. 1.16


I chose not to add regression tests trying to reproduce any specific
crash caused by the buffer overrun, because such crashes tended to
happen in the caller, typically roff_expand() in roff.c, or its
subroutine roff_expand_patch(), after they got handed invalid data
via the return arguments *rend and *rendarg, so whether or not,
and how, such invalid input caused crashes was strongly implementation-
dependent, hence not something that a regression suite could test for
with any reliability.

Yours,
  Ingo