Happy to contribute =)! And thanks for the review and feedback.
I attached a more complete patch for this bug. It applies to ‘branch-1.4’.
On Fri, Jul 8, 2022 at 2:10 PM Eric Blake <ebl...@redhat.com> wrote:
> On Fri, Jun 24, 2022 at 10:29:02PM -0600, Byron Johnson wrote:
> > Hello,
> >
> > I've attached to this email a patch that fixes a segfault from
> > ‘expand_user_macro’ so that integer overflows don't bypass the bounds
> > check. It applies to ‘branch-1.4’.
>
> Thanks for catching a lurking bug! The patch is not quite correct: by
> using unsigned, you have avoided the overflow to negative that
> triggered an out-of-bounds memory reference, but you did not prevent
> overflow where a macro definition of `$4294967297' is identical to a
> macro definition of `$1' whether or not your patch is applied. Better
> is to treat all cases of integer overflow as being larger than argc,
> and expand to an empty string, rather than having aliased expansions
> to earlier argument numbers, but that requires more than one line of
> code to do properly.
>
> >
> > Byron
>
> > From 1807c3bfca8ecb761f46be149dc3cb1ea2b041d2 Mon Sep 17 00:00:00 2001
> > From: Byron Johnson <by...@byronjohnson.net>
> > Date: Fri, 24 Jun 2022 21:59:35 -0600
> > Subject: [PATCH] Fix a macro expansion segfault from unchecked overflow.
> >
> > This example reproduces the bug on 1.4 m4's before this fix:
> > % ~/local/m4/1.4/bin/m4 <<< 'define(`mac'\'',
> $2028558489387014291456) mac'
> > /home/bairyn/local/m4/1.4/bin/m4: internal error detected; please
> report this bug to <bug...@gnu.org>: Segmentation fault
>
> That bug is ANCIENT! It is still present in commit bd11691d (ie, the
> very first git commit matching the release of 1.4 in Nov 1994); I have
> no access to sources specific to earlier release versions to know when
> the GNU extension of supporting $10 as the tenth parameter (rather
> than the first parameter concatenated with literal 0) was actually
> introduced, but that appears to be where the bug was introduced -
> perhaps as far back as release 0.50 in Jan 1990.
>
> Not every day you get to find and fix a bug that old!
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
>
From 8a61db622e7f42b4b4a77ac1cec4e8e5d3389cd5 Mon Sep 17 00:00:00 2001
From: Byron Johnson <by...@byronjohnson.net>
Date: Fri, 24 Jun 2022 21:59:35 -0600
Subject: [PATCH] Fix a macro expansion segfault from unchecked overflow.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This example reproduces the bug on 1.4 m4's before this fix:
% ~/local/m4/1.4/bin/m4 <<< 'define(`mac'\'', $2028558489387014291456) mac'
/home/bairyn/local/m4/1.4/bin/m4: internal error detected; please report this bug to <bug...@gnu.org>: Segmentation fault
* Fix the macro expansion overflow segfault.
* Also incorporate Eric Blake's feedback for more complete overflow checking,
rather than only fixing the segfault.
* Add tests for this that fail without this fix.
* Mention this fix in ‘NEWS’ and ‘THANKS’.
---
NEWS | 2 ++
THANKS | 1 +
doc/m4.texi | 12 ++++++++++++
src/builtin.c | 20 +++++++++++++++++++-
4 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index ee443ca5..53694702 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,8 @@ GNU M4 NEWS - User visible changes.
** The `syscmd' and `esyscmd' builtins no longer mishandle a command line
starting with `-' or `+'.
+** Fixed macro argument expansion overflow segfault.
+
* Noteworthy changes in release 1.4.19 (2021-05-28) [stable]
** A number of portability improvements inherited from gnulib, including
diff --git a/THANKS b/THANKS
index 861154f3..b9b077cc 100644
--- a/THANKS
+++ b/THANKS
@@ -24,6 +24,7 @@ Bob Badour b...@badour.net
Bob Proulx b...@proulx.com
Brendan Kehoe bren...@cygnus.com
Bruno Haible br...@clisp.org
+Byron Johnson by...@byronjohnson.net
Carlo Teubner carlo.teub...@gmail.com
Cesar Strauss cestra...@gmail.com
Chris McGuire ch...@wso.net
diff --git a/doc/m4.texi b/doc/m4.texi
index 99c248be..901049a4 100644
--- a/doc/m4.texi
+++ b/doc/m4.texi
@@ -1957,6 +1957,18 @@
access beyond the ninth argument, you can use the @code{argn} macro
documented later (@pxref{Shift}).
+@ignore
+@comment Detect macro expansion overflow segfault in 1.4.19.
+@comment Not worth including in the manual.
+@example
+define(`mac', `$2028558489387014291456')mac
+@result{}
+@example
+define(`mac', `$4294967297')mac(value)
+@result{}
+@end example
+@end ignore
+
POSIX also states that @samp{$} followed immediately by
@samp{@{} in a macro definition is implementation-defined. This version
of M4 passes the literal characters @samp{$@{} through unchanged, but M4
diff --git a/src/builtin.c b/src/builtin.c
index 0715a332..9dc68a2e 100644
--- a/src/builtin.c
+++ b/src/builtin.c
@@ -24,6 +24,8 @@
#include "m4.h"
+#include <limits.h>
+
#include "execute.h"
#include "memchr2.h"
#include "progname.h"
@@ -2248,7 +2250,23 @@ expand_user_macro (struct obstack *obs, symbol *sym,
else
{
for (i = 0; c_isdigit (*text); text++)
- i = i*10 + (*text - '0');
+ {
+ /* Check for overflow. (a*b > c iff a > floor (c/b)) */
+ int d = *text - '0';
+ if (i <= (INT_MAX - d)/10)
+ {
+ i = i*10 + d;
+ }
+ else
+ {
+ i = argc;
+ while (c_isdigit (*text))
+ {
+ text++;
+ }
+ break;
+ }
+ }
}
if (i < argc)
obstack_grow (obs, TOKEN_DATA_TEXT (argv[i]),
--
2.36.1
_______________________________________________
M4-patches mailing list
M4-patches@gnu.org
https://lists.gnu.org/mailman/listinfo/m4-patches
