Re: Patch request: Fix a macro expansion segfault from unchecked overflow.

--0000000000004611d805e3586c57
Content-Type: multipart/alternative; boundary="0000000000004611d605e3586c55"

--0000000000004611d605e3586c55
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Happy to contribute =3D)!  And thanks for the review and feedback.

I attached a more complete patch for this bug.  It applies to =E2=80=98bran=
ch-1.4=E2=80=99.

On Fri, Jul 8, 2022 at 2:10 PM Eric Blake <[email protected]> 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
> > =E2=80=98expand_user_macro=E2=80=99 so that integer overflows don't byp=
ass the bounds
> > check.  It applies to =E2=80=98branch-1.4=E2=80=99.
>
> 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 <[email protected]>
> > 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 <[email protected]>: 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
>
>

--0000000000004611d605e3586c55
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Happy to contribute =3D)!=C2=A0 And thanks for the review =
and feedback.<br><br>I attached a more complete patch for this bug.=C2=A0 I=
t applies to =E2=80=98branch-1.4=E2=80=99.<br></div><br><div class=3D"gmail=
_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Fri, Jul 8, 2022 at 2:10 P=
M Eric Blake &lt;<a href=3D"mailto:[email protected]">[email protected]</a>=
&gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px =
0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On F=
ri, Jun 24, 2022 at 10:29:02PM -0600, Byron Johnson wrote:<br>
&gt; Hello,<br>
&gt; <br>
&gt; I&#39;ve attached to this email a patch that fixes a segfault from<br>
&gt; =E2=80=98expand_user_macro=E2=80=99 so that integer overflows don&#39;=
t bypass the bounds<br>
&gt; check.=C2=A0 It applies to =E2=80=98branch-1.4=E2=80=99.<br>
<br>
Thanks for catching a lurking bug!=C2=A0 The patch is not quite correct: by=
<br>
using unsigned, you have avoided the overflow to negative that<br>
triggered an out-of-bounds memory reference, but you did not prevent<br>
overflow where a macro definition of `$4294967297&#39; is identical to a<br=
>
macro definition of `$1&#39; whether or not your patch is applied.=C2=A0 Be=
tter<br>
is to treat all cases of integer overflow as being larger than argc,<br>
and expand to an empty string, rather than having aliased expansions<br>
to earlier argument numbers, but that requires more than one line of<br>
code to do properly.<br>
<br>
&gt; <br>
&gt; Byron<br>
<br>
&gt; From 1807c3bfca8ecb761f46be149dc3cb1ea2b041d2 Mon Sep 17 00:00:00 2001=
<br>
&gt; From: Byron Johnson &lt;<a href=3D"mailto:[email protected]" targ=
et=3D"_blank">[email protected]</a>&gt;<br>
&gt; Date: Fri, 24 Jun 2022 21:59:35 -0600<br>
&gt; Subject: [PATCH] Fix a macro expansion segfault from unchecked overflo=
w.<br>
&gt; <br>
&gt; This example reproduces the bug on 1.4 m4&#39;s before this fix:<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0% ~/local/m4/1.4/bin/m4 &lt;&lt;&lt; &#39;de=
fine(`mac&#39;\&#39;&#39;, $2028558489387014291456) mac&#39;<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0/home/bairyn/local/m4/1.4/bin/m4: internal e=
rror detected; please report this bug to &lt;<a href=3D"mailto:[email protected]=
rg" target=3D"_blank">[email protected]</a>&gt;: Segmentation fault<br>
<br>
That bug is ANCIENT!=C2=A0 It is still present in commit bd11691d (ie, the<=
br>
very first git commit matching the release of 1.4 in Nov 1994); I have<br>
no access to sources specific to earlier release versions to know when<br>
the GNU extension of supporting $10 as the tenth parameter (rather<br>
than the first parameter concatenated with literal 0) was actually<br>
introduced, but that appears to be where the bug was introduced -<br>
perhaps as far back as release 0.50 in Jan 1990.<br>
<br>
Not every day you get to find and fix a bug that old!<br>
<br>
-- <br>
Eric Blake, Principal Software Engineer<br>
Red Hat, Inc.=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0+1-919-301-3266<br>
Virtualization:=C2=A0 <a href=3D"http://qemu.org" rel=3D"noreferrer" target=
=3D"_blank">qemu.org</a> | <a href=3D"http://libvirt.org" rel=3D"noreferrer=
" target=3D"_blank">libvirt.org</a><br>
<br>
</blockquote></div>

--0000000000004611d605e3586c55--

--0000000000004611d805e3586c57
Content-Type: text/x-patch; charset="UTF-8"; 
	name="0001-Fix-a-macro-expansion-segfault-from-unchecked-overfl.patch"
Content-Disposition: attachment; 
	filename="0001-Fix-a-macro-expansion-segfault-from-unchecked-overfl.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_l5dfbiru0>
X-Attachment-Id: f_l5dfbiru0

RnJvbSA4YTYxZGI2MjJlN2Y0MmI0YjRhNzdhYzFjZWM0ZThlNWQzMzg5Y2Q1IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBCeXJvbiBKb2huc29uIDxieXJvbkBieXJvbmpvaG5zb24ubmV0
PgpEYXRlOiBGcmksIDI0IEp1biAyMDIyIDIxOjU5OjM1IC0wNjAwClN1YmplY3Q6IFtQQVRDSF0g
Rml4IGEgbWFjcm8gZXhwYW5zaW9uIHNlZ2ZhdWx0IGZyb20gdW5jaGVja2VkIG92ZXJmbG93LgpN
SU1FLVZlcnNpb246IDEuMApDb250ZW50LVR5cGU6IHRleHQvcGxhaW47IGNoYXJzZXQ9VVRGLTgK
Q29udGVudC1UcmFuc2Zlci1FbmNvZGluZzogOGJpdAoKVGhpcyBleGFtcGxlIHJlcHJvZHVjZXMg
dGhlIGJ1ZyBvbiAxLjQgbTQncyBiZWZvcmUgdGhpcyBmaXg6CgklIH4vbG9jYWwvbTQvMS40L2Jp
bi9tNCA8PDwgJ2RlZmluZShgbWFjJ1wnJywgJDIwMjg1NTg0ODkzODcwMTQyOTE0NTYpIG1hYycK
CS9ob21lL2JhaXJ5bi9sb2NhbC9tNC8xLjQvYmluL200OiBpbnRlcm5hbCBlcnJvciBkZXRlY3Rl
ZDsgcGxlYXNlIHJlcG9ydCB0aGlzIGJ1ZyB0byA8YnVnLW00QGdudS5vcmc+OiBTZWdtZW50YXRp
b24gZmF1bHQKCiogRml4IHRoZSBtYWNybyBleHBhbnNpb24gb3ZlcmZsb3cgc2VnZmF1bHQuCiog
QWxzbyBpbmNvcnBvcmF0ZSBFcmljIEJsYWtlJ3MgZmVlZGJhY2sgZm9yIG1vcmUgY29tcGxldGUg
b3ZlcmZsb3cgY2hlY2tpbmcsCiAgcmF0aGVyIHRoYW4gb25seSBmaXhpbmcgdGhlIHNlZ2ZhdWx0
LgoqIEFkZCB0ZXN0cyBmb3IgdGhpcyB0aGF0IGZhaWwgd2l0aG91dCB0aGlzIGZpeC4KKiBNZW50
aW9uIHRoaXMgZml4IGluIOKAmE5FV1PigJkgYW5kIOKAmFRIQU5LU+KAmS4KLS0tCiBORVdTICAg
ICAgICAgIHwgIDIgKysKIFRIQU5LUyAgICAgICAgfCAgMSArCiBkb2MvbTQudGV4aSAgIHwgMTIg
KysrKysrKysrKysrCiBzcmMvYnVpbHRpbi5jIHwgMjAgKysrKysrKysrKysrKysrKysrKy0KIDQg
ZmlsZXMgY2hhbmdlZCwgMzQgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQoKZGlmZiAtLWdp
dCBhL05FV1MgYi9ORVdTCmluZGV4IGVlNDQzY2E1Li41MzY5NDcwMiAxMDA2NDQKLS0tIGEvTkVX
UworKysgYi9ORVdTCkBAIC01LDYgKzUsOCBAQCBHTlUgTTQgTkVXUyAtIFVzZXIgdmlzaWJsZSBj
aGFuZ2VzLgogKiogVGhlIGBzeXNjbWQnIGFuZCBgZXN5c2NtZCcgYnVpbHRpbnMgbm8gbG9uZ2Vy
IG1pc2hhbmRsZSBhIGNvbW1hbmQgbGluZQogICAgc3RhcnRpbmcgd2l0aCBgLScgb3IgYCsnLgog
CisqKiBGaXhlZCBtYWNybyBhcmd1bWVudCBleHBhbnNpb24gb3ZlcmZsb3cgc2VnZmF1bHQuCisK
ICogTm90ZXdvcnRoeSBjaGFuZ2VzIGluIHJlbGVhc2UgMS40LjE5ICgyMDIxLTA1LTI4KSBbc3Rh
YmxlXQogCiAqKiBBIG51bWJlciBvZiBwb3J0YWJpbGl0eSBpbXByb3ZlbWVudHMgaW5oZXJpdGVk
IGZyb20gZ251bGliLCBpbmNsdWRpbmcKZGlmZiAtLWdpdCBhL1RIQU5LUyBiL1RIQU5LUwppbmRl
eCA4NjExNTRmMy4uYjliMDc3Y2MgMTAwNjQ0Ci0tLSBhL1RIQU5LUworKysgYi9USEFOS1MKQEAg
LTI0LDYgKzI0LDcgQEAgQm9iIEJhZG91ciAgICAgICAgICAgICAgYm9iQGJhZG91ci5uZXQKIEJv
YiBQcm91bHggICAgICAgICAgICAgIGJvYkBwcm91bHguY29tCiBCcmVuZGFuIEtlaG9lICAgICAg
ICAgICBicmVuZGFuQGN5Z251cy5jb20KIEJydW5vIEhhaWJsZSAgICAgICAgICAgIGJydW5vQGNs
aXNwLm9yZworQnlyb24gSm9obnNvbiAgICAgICAgICAgYnlyb25AYnlyb25qb2huc29uLm5ldAog
Q2FybG8gVGV1Ym5lciAgICAgICAgICAgY2FybG8udGV1Ym5lckBnbWFpbC5jb20KIENlc2FyIFN0
cmF1c3MgICAgICAgICAgIGNlc3RyYXVzc0BnbWFpbC5jb20KIENocmlzIE1jR3VpcmUgICAgICAg
ICAgIGNocmlzQHdzby5uZXQKZGlmZiAtLWdpdCBhL2RvYy9tNC50ZXhpIGIvZG9jL200LnRleGkK
aW5kZXggOTljMjQ4YmUuLjkwMTA0OWE0IDEwMDY0NAotLS0gYS9kb2MvbTQudGV4aQorKysgYi9k
b2MvbTQudGV4aQpAQCAtMTk1Nyw2ICsxOTU3LDE4IEBACiBhY2Nlc3MgYmV5b25kIHRoZSBuaW50
aCBhcmd1bWVudCwgeW91IGNhbiB1c2UgdGhlIEBjb2Rle2FyZ259IG1hY3JvCiBkb2N1bWVudGVk
IGxhdGVyIChAcHhyZWZ7U2hpZnR9KS4KIAorQGlnbm9yZQorQGNvbW1lbnQgRGV0ZWN0IG1hY3Jv
IGV4cGFuc2lvbiBvdmVyZmxvdyBzZWdmYXVsdCBpbiAxLjQuMTkuCitAY29tbWVudCBOb3Qgd29y
dGggaW5jbHVkaW5nIGluIHRoZSBtYW51YWwuCitAZXhhbXBsZQorZGVmaW5lKGBtYWMnLCBgJDIw
Mjg1NTg0ODkzODcwMTQyOTE0NTYnKW1hYworQHJlc3VsdHt9CitAZXhhbXBsZQorZGVmaW5lKGBt
YWMnLCBgJDQyOTQ5NjcyOTcnKW1hYyh2YWx1ZSkKK0ByZXN1bHR7fQorQGVuZCBleGFtcGxlCitA
ZW5kIGlnbm9yZQorCiBQT1NJWCBhbHNvIHN0YXRlcyB0aGF0IEBzYW1weyR9IGZvbGxvd2VkIGlt
bWVkaWF0ZWx5IGJ5CiBAc2FtcHtAe30gaW4gYSBtYWNybyBkZWZpbml0aW9uIGlzIGltcGxlbWVu
dGF0aW9uLWRlZmluZWQuICBUaGlzIHZlcnNpb24KIG9mIE00IHBhc3NlcyB0aGUgbGl0ZXJhbCBj
aGFyYWN0ZXJzIEBzYW1weyRAe30gdGhyb3VnaCB1bmNoYW5nZWQsIGJ1dCBNNApkaWZmIC0tZ2l0
IGEvc3JjL2J1aWx0aW4uYyBiL3NyYy9idWlsdGluLmMKaW5kZXggMDcxNWEzMzIuLjlkYzY4YTJl
IDEwMDY0NAotLS0gYS9zcmMvYnVpbHRpbi5jCisrKyBiL3NyYy9idWlsdGluLmMKQEAgLTI0LDYg
KzI0LDggQEAKIAogI2luY2x1ZGUgIm00LmgiCiAKKyNpbmNsdWRlIDxsaW1pdHMuaD4KKwogI2lu
Y2x1ZGUgImV4ZWN1dGUuaCIKICNpbmNsdWRlICJtZW1jaHIyLmgiCiAjaW5jbHVkZSAicHJvZ25h
bWUuaCIKQEAgLTIyNDgsNyArMjI1MCwyMyBAQCBleHBhbmRfdXNlcl9tYWNybyAoc3RydWN0IG9i
c3RhY2sgKm9icywgc3ltYm9sICpzeW0sCiAgICAgICAgICAgZWxzZQogICAgICAgICAgICAgewog
ICAgICAgICAgICAgICBmb3IgKGkgPSAwOyBjX2lzZGlnaXQgKCp0ZXh0KTsgdGV4dCsrKQotICAg
ICAgICAgICAgICAgIGkgPSBpKjEwICsgKCp0ZXh0IC0gJzAnKTsKKyAgICAgICAgICAgICAgICB7
CisgICAgICAgICAgICAgICAgICAvKiBDaGVjayBmb3Igb3ZlcmZsb3cuICAoYSpiID4gYyBpZmYg
YSA+IGZsb29yIChjL2IpKSAqLworICAgICAgICAgICAgICAgICAgaW50IGQgPSAqdGV4dCAtICcw
JzsKKyAgICAgICAgICAgICAgICAgIGlmIChpIDw9IChJTlRfTUFYIC0gZCkvMTApCisgICAgICAg
ICAgICAgICAgICAgIHsKKyAgICAgICAgICAgICAgICAgICAgICBpID0gaSoxMCArIGQ7CisgICAg
ICAgICAgICAgICAgICAgIH0KKyAgICAgICAgICAgICAgICAgIGVsc2UKKyAgICAgICAgICAgICAg
ICAgICAgeworICAgICAgICAgICAgICAgICAgICAgIGkgPSBhcmdjOworICAgICAgICAgICAgICAg
ICAgICAgIHdoaWxlIChjX2lzZGlnaXQgKCp0ZXh0KSkKKyAgICAgICAgICAgICAgICAgICAgICAg
IHsKKyAgICAgICAgICAgICAgICAgICAgICAgICAgdGV4dCsrOworICAgICAgICAgICAgICAgICAg
ICAgICAgfQorICAgICAgICAgICAgICAgICAgICAgIGJyZWFrOworICAgICAgICAgICAgICAgICAg
ICB9CisgICAgICAgICAgICAgICAgfQogICAgICAgICAgICAgfQogICAgICAgICAgIGlmIChpIDwg
YXJnYykKICAgICAgICAgICAgIG9ic3RhY2tfZ3JvdyAob2JzLCBUT0tFTl9EQVRBX1RFWFQgKGFy
Z3ZbaV0pLAotLSAKMi4zNi4xCgo=
--0000000000004611d805e3586c57
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KTTQtcGF0Y2hl
cyBtYWlsaW5nIGxpc3QKTTQtcGF0Y2hlc0BnbnUub3JnCmh0dHBzOi8vbGlzdHMuZ251Lm9yZy9t
YWlsbWFuL2xpc3RpbmZvL200LXBhdGNoZXMK

--0000000000004611d805e3586c57--