[bug #68558] [PATCH] [troff] assertion failure in `do` request handler when given an invalid identifier

--8323329-424238335-1785301809=:3599353
Content-Type: TEXT/plain; CHARSET=utf-8
Content-Transfer-Encoding: QUOTED-PRINTABLE
Content-Disposition: inline

Follow-up Comment #4, bug #68558 (group groff):=0A=0AThanks, Dave.=0A=0AI u=
pdated the ChangeLog, but pushed before I remembered that I needed to take=
=0Ayour comment #3 into account.=0A=0AHere's the commit, but it is not corr=
ect.=0A=0A=0Acommit b3e0b25f085abf395a1fa7e8e139946fdf312106=0AAuthor: G. B=
randen Robinson <[email protected]>=0ADate:   Mon Jul 27 20:15:1=
5 2026 -0500=0A=0A    ChangeLog: Clarify Savannah #68558 defect history.=0A=
    =0A    The problem dates back to commit b04d345ed2 (groff 1.06), 1992-0=
9-01,=0A    which originally implemented the `do` request.  The request han=
dler=0A    disabled compatibility mode, then if it failed to read a valid=
=0A    identifier for any reason (invalid syntax, or no argument present),=
=0A    called `skip_line()`--meaning that the next input line would be=0A  =
  interpreted with compatibility mode disabled.  It did _not_ undo=0A    co=
mpatibility mode disablement and did not return early.=0A    =0A    Here's =
that original 1992 implementation.=0A    =0A    void do_request()=0A    {=
=0A      int saved_compatible_flag =3D compatible_flag;=0A      compatible_=
flag =3D 0;=0A      symbol nm =3D get_name();=0A      if (nm.is_null())=0A =
       skip_line();=0A      else=0A        interpolate_macro(nm);=0A      c=
ompatible_flag =3D saved_compatible_flag;=0A    }=0A    =0A    Since commit=
 205334d6e5, 2024-09-01, an argumentless `do` request no=0A    longer manip=
ulates compatibility mode, and since commit ba660bda6d,=0A    yesterday, th=
e request no longer does so for an invalid argument either.=0A=0A=0AAfter D=
ave's comment #3 I took a second look and now think that the bug came=0Ain =
with _groff_ 1.20.0.=0A=0AWe can see here that there is risk of a null poin=
ter being dereferenced.=0A=0A=0A$ git blame 1.20 -- src/roff/troff/input.cp=
p | sed -n 2619,2633p=0Ab04d345ed2 troff/input.cc           (James Clark   =
 1992-09-01 12:28:08 -0500=0A2619) void do_request()=0Ab04d345ed2 troff/inp=
ut.cc           (James Clark    1992-09-01 12:28:08 -0500=0A2620) {=0A9fbd2=
866b6 src/roff/troff/input.cc  (Werner LEMBERG 2001-05-06 23:29:21 +0000=0A=
2621)   int old_compatible_flag =3D compatible_flag;=0Ab04d345ed2 troff/inp=
ut.cc           (James Clark    1992-09-01 12:28:08 -0500=0A2622)   compati=
ble_flag =3D 0;=0Ab04d345ed2 troff/input.cc           (James Clark    1992-=
09-01 12:28:08 -0500=0A2623)   symbol nm =3D get_name();=0Ab04d345ed2 troff=
/input.cc           (James Clark    1992-09-01 12:28:08 -0500=0A2624)   if =
(nm.is_null())=0Ab04d345ed2 troff/input.cc           (James Clark    1992-0=
9-01 12:28:08 -0500=0A2625)     skip_line();=0Ab04d345ed2 troff/input.cc   =
        (James Clark    1992-09-01 12:28:08 -0500=0A2626)   else=0A6795d15c=
7b src/roff/troff/input.cpp (Werner LEMBERG 2008-09-25 07:47:38 +0000=0A262=
7)     interpolate_macro(nm, 1);=0A9fbd2866b6 src/roff/troff/input.cc  (Wer=
ner LEMBERG 2001-05-06 23:29:21 +0000=0A2628)   compatible_flag =3D old_com=
patible_flag;=0A6795d15c7b src/roff/troff/input.cpp (Werner LEMBERG 2008-09=
-25 07:47:38 +0000=0A2629)   request_or_macro *p =3D lookup_request(nm);=0A=
6795d15c7b src/roff/troff/input.cpp (Werner LEMBERG 2008-09-25 07:47:38 +00=
00=0A2630)   macro *m =3D p->to_macro();=0A6795d15c7b src/roff/troff/input.=
cpp (Werner LEMBERG 2008-09-25 07:47:38 +0000=0A2631)   if (m)=0A6795d15c7b=
 src/roff/troff/input.cpp (Werner LEMBERG 2008-09-25 07:47:38 +0000=0A2632)=
     tok.next();=0Ab04d345ed2 troff/input.cc           (James Clark    1992=
-09-01 12:28:08 -0500=0A2633) }=0A=0A=0ASee how control can flow from 2624 =
to 2628 if `nm.is_null()`, and thence to=0A2629.=0A=0AConsulting commit 679=
5d15c7b sheds much light on what was going on here.=0A=0A=0Acommit 6795d15c=
7bed7568853eb92c5334fb628b8110e2=0AAuthor: Werner LEMBERG <[email protected]>=0ADa=
te:   Thu Sep 25 07:47:38 2008 +0000=0A=0A    Fix incompatibility between `=
.de1' and `.do'.  Without this change,=0A    the following snippet=0A    =
=0A      .de1 xx=0A      .  tm \\n(.C=0A      ..=0A      .cp 1=0A      .do =
xx=0A    =0A    prints 1 instead of 0.=0A    =0A    * src/roff/troff/input.=
cc (do_request): If a macro gets processed,=0A    call tok.next().=0A    (i=
nterpolate_macro): Add optional argument.  Update callers.=0A    (request::=
invoke): Add optional argument.=0A    (macro::invoke): Add optional argumen=
t to delay call of tok.next().=0A    =0A    * src/roff/troff/request.h (req=
uest_or_macro): Add argument to=0A    `invoke' member.  Update all derived =
classes.=0A    =0A    * doc/groff.texinfo: Improve documentation of .do req=
uest.=0A=0Adiff --git a/ChangeLog b/ChangeLog=0Aindex 2f5cfa2ad..ed7a82585 =
100644=0A--- a/ChangeLog=0A+++ b/ChangeLog=0A@@ -1,3 +1,27 @@=0A+2008-09-24=
  Werner LEMBERG  <[email protected]>=0A+=0A+       Fix incompatibility between `.=
de1' and `.do'.  Without this change,=0A+       the following snippet=0A+=
=0A+         .de1 xx=0A+         .  tm \\n(.C=0A+         ..=0A+         .c=
p 1=0A+         .do xx=0A+=0A+       prints 1 instead of 0.=0A+=0A+       *=
 src/roff/troff/input.cc (do_request): If a macro gets processed,=0A+      =
 call tok.next().=0A+       (interpolate_macro): Add optional argument.  Up=
date callers.=0A+       (request::invoke): Add optional argument.=0A+      =
 (macro::invoke): Add optional argument to delay call of tok.next().=0A+=0A=
+       * src/roff/troff/request.h (request_or_macro): Add argument to=0A+ =
      `invoke' member.  Update all derived classes.=0A+=0A+       * doc/gro=
ff.texinfo: Improve documentation of .do request.=0A+=0A 2008-09-09  Werner=
 LEMBERG  <[email protected]>=0A =0A        * tmac/an-old.tmac (FT): Initialize pr=
operly.  Reported by Tadziu=0Adiff --git a/doc/groff.texinfo b/doc/groff.te=
xinfo=0Aindex 83bec1123..017ea32e1 100644=0A--- a/doc/groff.texinfo=0A+++ b=
/doc/groff.texinfo=0A@@ -14166,7 +14166,10 @@ @node Implementation Differen=
ces=0A option.=0A =0A The @code{do} request turns off compatibility mode=0A=
-while executing its arguments as a @code{gtroff} command.=0A+while executi=
ng its arguments as a @code{gtroff} command.  However, it=0A+does not turn =
off compatibility mode while processing the macro itself.=0A+To do that, us=
e the @code{de1} request (or manipulate the @code{.C}=0A+register manually)=
.  @xref{Writing Macros}.=0A =0A @Example=0A .do fam T=0Adiff --git a/src/r=
off/troff/input.cpp b/src/roff/troff/input.cpp=0Aindex bef9274bb..58e576b45=
 100644=0A--- a/src/roff/troff/input.cpp=0A+++ b/src/roff/troff/input.cpp=
=0A@@ -139,7 +139,7 @@ static symbol read_escape_name(read_mode =3D NO_ARGS=
);=0A static symbol read_long_escape_name(read_mode =3D NO_ARGS);=0A static=
 void interpolate_string(symbol);=0A static void interpolate_string_with_ar=
gs(symbol);=0A-static void interpolate_macro(symbol);=0A+static void interp=
olate_macro(symbol, int =3D 0);=0A static void interpolate_number_format(sy=
mbol);=0A static void interpolate_environment_variable(symbol);=0A =0A@@ -2=
601,8 +2601,12 @@ void do_request()=0A   if (nm.is_null())=0A     skip_line=
();=0A   else=0A-    interpolate_macro(nm);=0A+    interpolate_macro(nm, 1)=
;=0A   compatible_flag =3D old_compatible_flag;=0A+  request_or_macro *p =
=3D lookup_request(nm);=0A+  macro *m =3D p->to_macro();=0A+  if (m)=0A+   =
 tok.next();=0A }=0A =0A inline int possibly_handle_first_page_transition()=
=0A@@ -3006,7 +3010,7 @@ request::request(REQUEST_FUNCP pp) : p(pp)=0A {=0A=
 }=0A =0A-void request::invoke(symbol)=0A+void request::invoke(symbol, int)=
=0A {=0A   (*p)();=0A }=0A@@ -3716,7 +3720,7 @@ int operator=3D=3D(const ma=
cro &m1, const macro &m2)=0A   return 1;=0A }=0A =0A-static void interpolat=
e_macro(symbol nm)=0A+static void interpolate_macro(symbol nm, int no_next)=
=0A {=0A   request_or_macro *p =3D (request_or_macro *)request_dictionary.l=
ookup(nm);=0A   if (p =3D=3D 0) {=0A@@ -3745,7 +3749,7 @@ static void inter=
polate_macro(symbol nm)=0A     }=0A   }=0A   if (p)=0A-    p->invoke(nm);=
=0A+    p->invoke(nm, no_next);=0A   else {=0A     skip_line();=0A     retu=
rn;=0A@@ -3854,12 +3858,15 @@ static void decode_string_args(macro_iterator=
 *mi)=0A   }=0A }=0A =0A-void macro::invoke(symbol nm)=0A+void macro::invok=
e(symbol nm, int no_next)=0A {=0A   macro_iterator *mi =3D new macro_iterat=
or(nm, *this);=0A   decode_args(mi);=0A   input_stack::push(mi);=0A-  tok.n=
ext();=0A+  // we must delay tok.next() in case the function has been calle=
d by=0A+  // do_request to assure proper handling of compatible_flag=0A+  i=
f (!no_next)=0A+    tok.next();=0A }=0A =0A macro *macro::to_macro()=0Adiff=
 --git a/src/roff/troff/request.h b/src/roff/troff/request.h=0Aindex 24d258=
90f..098de7cc1 100644=0A--- a/src/roff/troff/request.h=0A+++ b/src/roff/tro=
ff/request.h=0A@@ -1,5 +1,5 @@=0A // -*- C++ -*-=0A-/* Copyright (C) 1989, =
1990, 1991, 1992, 2000, 2001, 2002, 2004=0A+/* Copyright (C) 1989, 1990, 19=
91, 1992, 2000, 2001, 2002, 2004, 2008=0A    Free Software Foundation, Inc.=
=0A      Written by James Clark ([email protected])=0A =0A@@ -26,14 +26,14 @@ =
class macro;=0A class request_or_macro : public object {=0A public:=0A   re=
quest_or_macro();=0A-  virtual void invoke(symbol s) =3D 0;=0A+  virtual vo=
id invoke(symbol, int) =3D 0;=0A   virtual macro *to_macro();=0A };=0A =0A =
class request : public request_or_macro {=0A   REQUEST_FUNCP p;=0A public:=
=0A-  void invoke(symbol);=0A+  void invoke(symbol, int);=0A   request(REQU=
EST_FUNCP);=0A };=0A =0A@@ -59,13 +59,13 @@ public:=0A   macro &operator=3D=
(const macro &);=0A   void append(unsigned char);=0A   void append(node *);=
=0A-  void append_unsigned(unsigned int i);=0A-  void append_int(int i);=0A=
+  void append_unsigned(unsigned int);=0A+  void append_int(int);=0A   void=
 append_str(const char *);=0A   void set(unsigned char, int);=0A   unsigned=
 char get(int);=0A   int length();=0A-  void invoke(symbol);=0A+  void invo=
ke(symbol, int);=0A   macro *to_macro();=0A   void print_size();=0A   int e=
mpty();=0A@@ -83,7 +83,7 @@ extern void init_node_requests();=0A extern voi=
d init_reg_requests();=0A extern void init_env_requests();=0A extern void i=
nit_hyphen_requests();=0A-extern void init_request(const char *s, REQUEST_F=
UNCP f);=0A+extern void init_request(const char *, REQUEST_FUNCP);=0A =0A c=
lass charinfo;=0A class environment;=0A=0A=0AI think I want to refactor thi=
s to eliminate this `int no_next` function=0Aparameter.  (It's now called `=
do_not_want_next_token`.)=0A=0AI have an idea for an alternative solution, =
already annotated in the source.=0A=0A=0A$ sed -n 9539,9584p src/roff/troff=
/input.cpp =0A=0A// Consume the rest of the input line in copy mode and ret=
urn it as a C=0A// string; if, after spaces, the argument starts with a `"`=
, discard it,=0A// letting any immediately subsequent spaces populate the r=
eturned=0A// string.=0A//=0A// The caller must subsequently call `tok.next(=
)`, _not_ `skip_line()`,=0A// to advance the input stream pointer.=0A//=0A/=
/ TODO: Synthesize a newline token and push it onto the input stream so=0A/=
/ that the foregoing becomes unnecessary, and request handlers can=0A// uni=
formly use `skip_line()`.=0A//=0A// The caller has responsibility for `dele=
te`ing the returned buffer.=0Achar *read_rest_of_line_as_argument()=0A{=0A =
 int buf_size =3D 256;=0A  char *s =3D new char[buf_size]; // C++03: new ch=
ar[buf_size]();=0A  (void) memset(s, 0, (buf_size * sizeof(char)));=0A  int=
 c =3D read_character_in_copy_mode(0 /* nullptr */);=0A  while (' ' =3D=3D =
c)=0A    c =3D read_character_in_copy_mode(0 /* nullptr */);=0A  if ('"' =
=3D=3D c)=0A    c =3D read_character_in_copy_mode(0 /* nullptr */);=0A  int=
 i =3D 0;=0A  while ((c !=3D '\n') && (c !=3D EOF)) {=0A    if (!is_invalid=
_input_char(c)) {=0A      if ((i + 2) > buf_size) {=0A        char *tem =3D=
 s;=0A        s =3D new char[buf_size * 2]; // C++03: new char[buf_size * 2=
]();=0A        (void) memset(s, 0, (buf_size * 2 * sizeof(char)));=0A      =
  memcpy(s, tem, buf_size);=0A        buf_size *=3D 2;=0A        delete[] t=
em;=0A      }=0A      s[i++] =3D c;=0A    }=0A    c =3D read_character_in_c=
opy_mode(0 /* nullptr */);=0A  }=0A  s[i] =3D '\0';=0A  if (0 =3D=3D i) {=
=0A    delete[] s;=0A    return 0 /* nullptr */;=0A  }=0A  return s;=0A}=0A=
=0A=0AI furthermore think that implementing this newline synthesis + unifor=
m=0A`skip_line()` approach will fix bug #44714.  I don't think the diagnosi=
s there=0Ais correct--I don't think compatibility mode management is implic=
ated at all.=0A(We now handle compatibility mode with with a proper STL sta=
ck of `bool`, and=0Adoing so didn't fix that problem.)=0A=0AI look forward =
to attempting to prove my conjectures in the _groff_ 1.26=0Adevelopment cyc=
le.=0A=0AAn issue still to be dealt with is request handlers that read argu=
ments in=0Acopy mode that _don't_ consume the rest of the input line.  I ne=
ed to see if=0Athere are any, and if so, they need to synthesize a newline =
and push it onto=0Athe token stream, but only if such an argument is the la=
st thing on the input=0Aline.  (They could be followed by ignored "extra" a=
rguments, or a comment=0Aescape sequence.)=0A=0A=0A    ____________________=
___________________________________=0A=0AReply to this item at:=0A=0A  <htt=
ps://savannah.gnu.org/bugs/?68558>=0A=0A___________________________________=
____________=0AMessage sent via Savannah=0Ahttps://savannah.gnu.org/=0A
--8323329-424238335-1785301809=:3599353
Content-Type: APPLICATION/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQQk97aszIMMAvLLwm6qLAuaBUf3TgUCammLMQAKCRCqLAuaBUf3
ToPdAQC8je3fgsRTYyQHx+boSPQ2BgGZQWQ9s9/yZ4YlfK4magD7BinF+arjh2WJ
gxKef4cNxy6vh3wu24hh7dvb2DulFws=
=W/Mu
-----END PGP SIGNATURE-----

--8323329-424238335-1785301809=:3599353--