[bug #68544] [pic] heap buffer overflow via large macro argument index
"G. Branden Robinson" <[email protected]> Fri, 31 Jul 2026 17:55:33 -0400 (EDT)
| Newsgroups | gmane.comp.printing.groff.bugs |
|---|---|
| Message-ID | <[email protected]> |
--8323329-1649760492-1785534933=:3267507
Content-Type: TEXT/plain; CHARSET=utf-8
Content-Transfer-Encoding: QUOTED-PRINTABLE
Content-Disposition: inline
Update of bug #68544 (group groff):=0A=0A Severity: =
3 - Normal =3D> 4 - Important=0A Status: =
Need Info =3D> Fixed=0A Assigned to: None =
=3D> gbranden=0A Open/Closed: Open =3D> Clos=
ed=0A Planned Release: None =3D> 1.25.0=0A=0A =
_______________________________________________________=0A=0AFollow-up Com=
ment #4:=0A=0A`git bisect` confirms that I fixed this almost 2 months ago.=
=0A=0A=0Acommit 830e119147b4dc05d42f6d08a3dd653f047e278b=0AAuthor: G. Brand=
en Robinson <[email protected]>=0ADate: Thu Jun 4 13:50:43 202=
6 -0500=0A=0A [libgroff]: Clear heap memory backing `string`s.=0A =0A=
* src/libs/libgroff/string.cpp (salloc, sfree_alloc, srealloc)=0A =
(string::string, string::clear, string::remove_spaces): Zero out=0A me=
mory allocations from the heap, and when user requests erasure of=0A `=
string` contents.=0A =0A This change resolves an issue I noted when i=
nstrumenting grohtml(1) to=0A inspect the groff `string`s and C strings =
it passes around internally.=0A =0A Examples of differences using `de=
bug()` instrumentation in my working=0A copy:=0A =0A $ diff -U0 ER=
R1 ERR2|cat -v|head -n 27=0A --- ERR1 2026-06-03 22:55:51.9046924=
49 -0500=0A +++ ERR2 2026-06-03 22:57:18.775831578 -0500=0A @@=
-23,12 +23,12 @@=0A -post-grohtml:<standard input>:(www.tmac):57: debug=
: GBR:=0Ahtml_printer::flush_sbuf() flushing 'Home^L^?'=0A -post-grohtml=
:<standard input>:(www.tmac):57: debug: GBR: page::add()=0A'Home^L^?'=0A =
-post-grohtml:<standard input>:(www.tmac):57: debug: GBR:=0Achar_buffer::a=
dd_string() adding 'Home^L^?'=0A -post-grohtml:<standard input>:(www.tma=
c):59: debug: GBR:=0Ahtml_printer::flush_sbuf() flushing 'ofme^L^?'=0A -=
post-grohtml:<standard input>:(www.tmac):59: debug: GBR: page::add()=0A'ofm=
e^L^?'=0A -post-grohtml:<standard input>:(www.tmac):59: debug: GBR:=0Ach=
ar_buffer::add_string() adding 'ofme^L^?'=0A -post-grohtml:<standard inp=
ut>:(www.tmac):61: debug: GBR:=0Ahtml_printer::flush_sbuf() flushing 'Groff=
^?'=0A -post-grohtml:<standard input>:(www.tmac):61: debug: GBR: page::a=
dd()=0A'Groff^?'=0A -post-grohtml:<standard input>:(www.tmac):61: debug:=
GBR:=0Achar_buffer::add_string() adding 'Groff^?'=0A -post-grohtml:<sta=
ndard input>:(www.tmac):63: debug: GBR:=0Ahtml_printer::flush_sbuf() flushi=
ng '(GNUf^?'=0A -post-grohtml:<standard input>:(www.tmac):63: debug: GBR=
: page::add()=0A'(GNUf^?'=0A -post-grohtml:<standard input>:(www.tmac):6=
3: debug: GBR:=0Achar_buffer::add_string() adding '(GNUf^?'=0A +post-gro=
html:<standard input>:(www.tmac):57: debug: GBR:=0Ahtml_printer::flush_sbuf=
() flushing 'Home'=0A +post-grohtml:<standard input>:(www.tmac):57: debu=
g: GBR: page::add()=0A'Home'=0A +post-grohtml:<standard input>:(www.tmac=
):57: debug: GBR:=0Achar_buffer::add_string() adding 'Home'=0A +post-gro=
html:<standard input>:(www.tmac):59: debug: GBR:=0Ahtml_printer::flush_sbuf=
() flushing 'of'=0A +post-grohtml:<standard input>:(www.tmac):59: debug:=
GBR: page::add()=0A'of'=0A +post-grohtml:<standard input>:(www.tmac):59=
: debug: GBR:=0Achar_buffer::add_string() adding 'of'=0A +post-grohtml:<=
standard input>:(www.tmac):61: debug: GBR:=0Ahtml_printer::flush_sbuf() flu=
shing 'Groff'=0A +post-grohtml:<standard input>:(www.tmac):61: debug: GB=
R: page::add()=0A'Groff'=0A +post-grohtml:<standard input>:(www.tmac):61=
: debug: GBR:=0Achar_buffer::add_string() adding 'Groff'=0A +post-grohtm=
l:<standard input>:(www.tmac):63: debug: GBR:=0Ahtml_printer::flush_sbuf() =
flushing '(GNU'=0A +post-grohtml:<standard input>:(www.tmac):63: debug: =
GBR: page::add()=0A'(GNU'=0A +post-grohtml:<standard input>:(www.tmac):6=
3: debug: GBR:=0Achar_buffer::add_string() adding '(GNU'=0A =0A There=
are worse examples than the foregoing; the copying of unnecessary=0A by=
tes can cause reallocations from the heap as the `char_buffer` grows=0A =
to accommodate string content that won't be used.=0A =0A $ diff -U0 E=
RR1 ERR2|cat -v|grep -w 640=0A -post-grohtml:<standard input>:(www.tmac)=
:640: debug: GBR:=0Ahtml_printer::flush_sbuf() flushing 'topgecededonslyves=
”)g/">-1.png'=0A -post-grohtml:<standard input>:(www.tmac):640: de=
bug: GBR: page::add()=0A'topgecededonslyves”)g/">-1.png'=0A -post-=
grohtml:<standard input>:(www.tmac):640: debug: GBR:=0Achar_buffer::add_str=
ing() adding 'topgecededonslyves”)g/">-1.png'=0A +post-grohtml:<st=
andard input>:(www.tmac):640: debug: GBR:=0Ahtml_printer::flush_sbuf() flus=
hing 'top'=0A +post-grohtml:<standard input>:(www.tmac):640: debug: GBR:=
page::add()=0A'top'=0A +post-grohtml:<standard input>:(www.tmac):640: d=
ebug: GBR:=0Achar_buffer::add_string() adding 'top'=0A =0A Here's ano=
ther case, a more obnoxious one because the excess content of=0A these s=
trings could cause unnecessary memory (re)allocations.=0A =0A $ diff =
-U0 ERR1 ERR2|cat -v|grep -w 36409=0A -post-grohtml:<standard input>:(<s=
tandard input>):36409: debug: GBR:=0Ahtml_printer::flush_sbuf() flushing 'A=
UMBERr.s:deor,t;;;t;uot;;;;ot;0)'=0A -post-grohtml:<standard input>:(<st=
andard input>):36409: debug: GBR:=0Apage::add() 'AUMBERr.s:deor,t;;;t;uot;;=
;;ot;0)'=0A -post-grohtml:<standard input>:(<standard input>):36409: deb=
ug: GBR:=0Achar_buffer::add_string() adding 'AUMBERr.s:deor,t;;;t;uot;;;;ot=
;0)'=0A +post-grohtml:<standard input>:(<standard input>):36409: debug: =
GBR:=0Ahtml_printer::flush_sbuf() flushing 'A'=0A +post-grohtml:<standar=
d input>:(<standard input>):36409: debug: GBR:=0Apage::add() 'A'=0A +pos=
t-grohtml:<standard input>:(<standard input>):36409: debug: GBR:=0Achar_buf=
fer::add_string() adding 'A'=0A =0A Performance analysis=0A =3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A =0A Insigni=
ficant change.=0A =0A $ bash ./measure-all-docs.bash | datamash range=
1 mean 1 sstdev 1=0A =0A Before:=0A 1.18 24.1655 0.22933485056=
618=0A =0A After:=0A 1.41 24.1565 0.32841365123239=0A =0A =
$ cat ./measure-all-docs.bash=0A \#!/bin/bash=0A =0A \# Example:=
=0A \# bash ./measure-all-docs.bash | datamash range 1 mean 1 sstdev 1=
=0A =0A shopt -s extglob=0A =0A if ! make -C build -j >/dev/nul=
l 2>&1=0A then=0A echo "$0: fatal error; tree is not buildabl=
e" >&2=0A exit 1=0A fi=0A =0A : ${N:=3D20}=0A =0A =
for n in $(seq $N)=0A do=0A printf "%d " $n >&2=0A =
rm -f build/doc/!(groff).pdf build/doc/*.ps \=0A build=
/doc/!(groff).html \=0A build/contrib/hdtbl/examples/*.p=
s \=0A build/contrib/mom/examples/*.pdf \=0A =
contrib/sboxes/msboxes.pdf \=0A doc/groff-man-pa=
ges.utf8.txt=0A t=3D$({ time -p make -C build >/dev/null 2>&1; }=
2>/dev/stdout)=0A echo "$t" | awk '/real/ { print $2; }'=0A =
done=0A =0A echo >&2=0A=0Adiff --git a/ChangeLog b/ChangeLog=0Aindex =
dfc0e1d4c..42dd64044 100644=0A--- a/ChangeLog=0A+++ b/ChangeLog=0A@@ -1,3 +=
1,10 @@=0A+2026-06-04 G. Branden Robinson <[email protected]>=
=0A+=0A+ * src/libs/libgroff/string.cpp (salloc, sfree_alloc, sreallo=
c)=0A+ (string::string, string::clear, string::remove_spaces): Zero o=
ut=0A+ memory allocations from the heap, and when user requests erasu=
re=0A+ of `string` contents.=0A+=0A 2026-06-04 G. Branden Robinson <=
[email protected]>=0A =0A * src/libs/libgroff/string.cpp:=
Fix code style nits.=0Adiff --git a/src/libs/libgroff/string.cpp b/src/lib=
s/libgroff/string.cpp=0Aindex 2d31c43e8..c735548af 100644=0A--- a/src/libs/=
libgroff/string.cpp=0A+++ b/src/libs/libgroff/string.cpp=0A@@ -44,8 +44,12 =
@@ static char *salloc(int len, int *sizep)=0A *sizep =3D 0;=0A ret=
urn 0 /* nullptr */;=0A }=0A- else=0A- return new char[*sizep =3D (le=
n * 2)];=0A+ char *p =3D 0 /* nullptr */;=0A+ size_t amount =3D len * 2;=
=0A+ p =3D new char[*sizep =3D amount];=0A+ assert(amount > 0);=0A+ mems=
et(p, 0, amount);=0A+ return p;=0A }=0A =0A static void sfree(char *ptr, i=
nt)=0A@@ -64,8 +68,12 @@ static char *sfree_alloc(char *ptr, int oldsz, int=
len, int=0A*sizep)=0A *sizep =3D 0;=0A return 0 /* nullptr */;=0A =
}=0A- else=0A- return new char[*sizep =3D (len * 2)];=0A+ char *p =
=3D 0 /* nullptr */;=0A+ size_t amount =3D len * 2;=0A+ p =3D new char[*s=
izep =3D amount];=0A+ assert(amount > 0);=0A+ memset(p, 0, amount);=0A+ =
return p;=0A }=0A =0A static char *srealloc(char *ptr, int oldsz, int oldle=
n, int newlen,=0A@@ -81,9 +89,14 @@ static char *srealloc(char *ptr, int ol=
dsz, int oldlen, int=0Anewlen,=0A return 0 /* nullptr */;=0A }=0A e=
lse {=0A- char *p =3D new char[*sizep =3D (newlen * 2)];=0A- if ((old=
len < newlen) && (oldlen !=3D 0))=0A+ size_t amount =3D newlen * 2;=0A+ =
char *p =3D 0 /* nullptr */;=0A+ p =3D new char[*sizep =3D amount];=
=0A+ if ((oldlen < newlen) && (oldlen !=3D 0)) {=0A+ assert(amount =
> 0);=0A+ memset(p, 0, amount);=0A memcpy(p, ptr, oldlen);=0A+ =
}=0A delete[] ptr;=0A return p;=0A }=0A@@ -97,6 +110,8 @@ strin=
g::string(const char *p, int n) : len(n)=0A {=0A assert(n >=3D 0);=0A p=
tr =3D salloc(n, &sz);=0A+ if (sz > 0)=0A+ memset(ptr, 0, sz);=0A if =
(n !=3D 0)=0A memcpy(ptr, p, n);=0A }=0A@@ -107,15 +122,14 @@ string::s=
tring(const char *p)=0A len =3D 0;=0A ptr =3D 0 /* nullptr */;=0A =
sz =3D 0;=0A+ return;=0A }=0A- else {=0A- len =3D strlen(p);=0A=
- ptr =3D salloc(len, &sz);=0A- if (len < sz)=0A- memset(ptr, 0,=
sz);=0A- if (len !=3D 0)=0A- memcpy(ptr, p, len);=0A- }=0A+ len =
=3D strlen(p);=0A+ ptr =3D salloc(len, &sz);=0A+ if ((sz > 0) && (len < s=
z))=0A+ memset(ptr, 0, sz);=0A+ if (len !=3D 0)=0A+ memcpy(ptr, p, l=
en);=0A }=0A =0A string::string(char c) : len(1)=0A@@ -127,6 +141,8 @@ stri=
ng::string(char c) : len(1)=0A string::string(const string &s) : len(s.len)=
=0A {=0A ptr =3D salloc(len, &sz);=0A+ if (sz > 0)=0A+ memset(ptr, 0,=
sz);=0A if (len !=3D 0)=0A memcpy(ptr, s.ptr, len);=0A }=0A@@ -281,6=
+297,8 @@ void string::set_length(int i)=0A =0A void string::clear()=0A {=
=0A+ if (ptr !=3D 0 /* nullptr */)=0A+ memset(ptr, 0, sz);=0A len =3D=
0;=0A }=0A =0A@@ -414,7 +432,10 @@ void string::remove_spaces()=0A if (l=
en - 1 !=3D l) {=0A if (l >=3D 0) {=0A len =3D l + 1;=0A- ch=
ar *tmp =3D new char[sz];=0A+ char *tmp =3D 0 /* nullptr */;=0A+ =
tmp =3D new char[sz];=0A+ assert(sz > 0);=0A+ memset(tmp, 0, sz);=
=0A memcpy(tmp, p, len);=0A delete[] ptr;=0A ptr =3D tmp;=
=0A=0A=0A=0A=0A _______________________________________________________=
=0A=0AReply to this item at:=0A=0A <https://savannah.gnu.org/bugs/?68544>=
=0A=0A_______________________________________________=0AMessage sent via Sa=
vannah=0Ahttps://savannah.gnu.org/=0A
--8323329-1649760492-1785534933=:3267507
Content-Type: APPLICATION/pgp-signature; name=signature.asc
-----BEGIN PGP SIGNATURE-----
iHUEABYIAB0WIQQk97aszIMMAvLLwm6qLAuaBUf3TgUCam0Z1QAKCRCqLAuaBUf3
TuJYAQCBoJtBr59gk9IPFNHia/CGei4NDz0RNeN4r3j6gNAf3wD+Izj8sgX9LKDo
TgHGgCyM7Q74Z2tI0peqeCKtn3niJwU=
=qlCv
-----END PGP SIGNATURE-----
--8323329-1649760492-1785534933=:3267507--