Follow-up Comment #4, bug #68558 (group groff): Thanks, Dave.
I updated the ChangeLog, but pushed before I remembered that I needed to take your comment #3 into account. Here's the commit, but it is not correct. commit b3e0b25f085abf395a1fa7e8e139946fdf312106 Author: G. Branden Robinson <[email protected]> Date: Mon Jul 27 20:15:15 2026 -0500 ChangeLog: Clarify Savannah #68558 defect history. The problem dates back to commit b04d345ed2 (groff 1.06), 1992-09-01, which originally implemented the `do` request. The request handler disabled compatibility mode, then if it failed to read a valid identifier for any reason (invalid syntax, or no argument present), called `skip_line()`--meaning that the next input line would be interpreted with compatibility mode disabled. It did _not_ undo compatibility mode disablement and did not return early. Here's that original 1992 implementation. void do_request() { int saved_compatible_flag = compatible_flag; compatible_flag = 0; symbol nm = get_name(); if (nm.is_null()) skip_line(); else interpolate_macro(nm); compatible_flag = saved_compatible_flag; } Since commit 205334d6e5, 2024-09-01, an argumentless `do` request no longer manipulates compatibility mode, and since commit ba660bda6d, yesterday, the request no longer does so for an invalid argument either. After Dave's comment #3 I took a second look and now think that the bug came in with _groff_ 1.20.0. We can see here that there is risk of a null pointer being dereferenced. $ git blame 1.20 -- src/roff/troff/input.cpp | sed -n 2619,2633p b04d345ed2 troff/input.cc (James Clark 1992-09-01 12:28:08 -0500 2619) void do_request() b04d345ed2 troff/input.cc (James Clark 1992-09-01 12:28:08 -0500 2620) { 9fbd2866b6 src/roff/troff/input.cc (Werner LEMBERG 2001-05-06 23:29:21 +0000 2621) int old_compatible_flag = compatible_flag; b04d345ed2 troff/input.cc (James Clark 1992-09-01 12:28:08 -0500 2622) compatible_flag = 0; b04d345ed2 troff/input.cc (James Clark 1992-09-01 12:28:08 -0500 2623) symbol nm = get_name(); b04d345ed2 troff/input.cc (James Clark 1992-09-01 12:28:08 -0500 2624) if (nm.is_null()) b04d345ed2 troff/input.cc (James Clark 1992-09-01 12:28:08 -0500 2625) skip_line(); b04d345ed2 troff/input.cc (James Clark 1992-09-01 12:28:08 -0500 2626) else 6795d15c7b src/roff/troff/input.cpp (Werner LEMBERG 2008-09-25 07:47:38 +0000 2627) interpolate_macro(nm, 1); 9fbd2866b6 src/roff/troff/input.cc (Werner LEMBERG 2001-05-06 23:29:21 +0000 2628) compatible_flag = old_compatible_flag; 6795d15c7b src/roff/troff/input.cpp (Werner LEMBERG 2008-09-25 07:47:38 +0000 2629) request_or_macro *p = lookup_request(nm); 6795d15c7b src/roff/troff/input.cpp (Werner LEMBERG 2008-09-25 07:47:38 +0000 2630) macro *m = p->to_macro(); 6795d15c7b src/roff/troff/input.cpp (Werner LEMBERG 2008-09-25 07:47:38 +0000 2631) if (m) 6795d15c7b src/roff/troff/input.cpp (Werner LEMBERG 2008-09-25 07:47:38 +0000 2632) tok.next(); b04d345ed2 troff/input.cc (James Clark 1992-09-01 12:28:08 -0500 2633) } See how control can flow from 2624 to 2628 if `nm.is_null()`, and thence to 2629. Consulting commit 6795d15c7b sheds much light on what was going on here. commit 6795d15c7bed7568853eb92c5334fb628b8110e2 Author: Werner LEMBERG <[email protected]> Date: Thu Sep 25 07:47:38 2008 +0000 Fix incompatibility between `.de1' and `.do'. Without this change, the following snippet .de1 xx . tm \\n(.C .. .cp 1 .do xx prints 1 instead of 0. * src/roff/troff/input.cc (do_request): If a macro gets processed, call tok.next(). (interpolate_macro): Add optional argument. Update callers. (request::invoke): Add optional argument. (macro::invoke): Add optional argument to delay call of tok.next(). * src/roff/troff/request.h (request_or_macro): Add argument to `invoke' member. Update all derived classes. * doc/groff.texinfo: Improve documentation of .do request. diff --git a/ChangeLog b/ChangeLog index 2f5cfa2ad..ed7a82585 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,27 @@ +2008-09-24 Werner LEMBERG <[email protected]> + + Fix incompatibility between `.de1' and `.do'. Without this change, + the following snippet + + .de1 xx + . tm \\n(.C + .. + .cp 1 + .do xx + + prints 1 instead of 0. + + * src/roff/troff/input.cc (do_request): If a macro gets processed, + call tok.next(). + (interpolate_macro): Add optional argument. Update callers. + (request::invoke): Add optional argument. + (macro::invoke): Add optional argument to delay call of tok.next(). + + * src/roff/troff/request.h (request_or_macro): Add argument to + `invoke' member. Update all derived classes. + + * doc/groff.texinfo: Improve documentation of .do request. + 2008-09-09 Werner LEMBERG <[email protected]> * tmac/an-old.tmac (FT): Initialize properly. Reported by Tadziu diff --git a/doc/groff.texinfo b/doc/groff.texinfo index 83bec1123..017ea32e1 100644 --- a/doc/groff.texinfo +++ b/doc/groff.texinfo @@ -14166,7 +14166,10 @@ @node Implementation Differences option. The @code{do} request turns off compatibility mode -while executing its arguments as a @code{gtroff} command. +while executing its arguments as a @code{gtroff} command. However, it +does not turn off compatibility mode while processing the macro itself. +To do that, use the @code{de1} request (or manipulate the @code{.C} +register manually). @xref{Writing Macros}. @Example .do fam T diff --git a/src/roff/troff/input.cpp b/src/roff/troff/input.cpp index bef9274bb..58e576b45 100644 --- a/src/roff/troff/input.cpp +++ b/src/roff/troff/input.cpp @@ -139,7 +139,7 @@ static symbol read_escape_name(read_mode = NO_ARGS); static symbol read_long_escape_name(read_mode = NO_ARGS); static void interpolate_string(symbol); static void interpolate_string_with_args(symbol); -static void interpolate_macro(symbol); +static void interpolate_macro(symbol, int = 0); static void interpolate_number_format(symbol); static void interpolate_environment_variable(symbol); @@ -2601,8 +2601,12 @@ void do_request() if (nm.is_null()) skip_line(); else - interpolate_macro(nm); + interpolate_macro(nm, 1); compatible_flag = old_compatible_flag; + request_or_macro *p = lookup_request(nm); + macro *m = p->to_macro(); + if (m) + tok.next(); } inline int possibly_handle_first_page_transition() @@ -3006,7 +3010,7 @@ request::request(REQUEST_FUNCP pp) : p(pp) { } -void request::invoke(symbol) +void request::invoke(symbol, int) { (*p)(); } @@ -3716,7 +3720,7 @@ int operator==(const macro &m1, const macro &m2) return 1; } -static void interpolate_macro(symbol nm) +static void interpolate_macro(symbol nm, int no_next) { request_or_macro *p = (request_or_macro *)request_dictionary.lookup(nm); if (p == 0) { @@ -3745,7 +3749,7 @@ static void interpolate_macro(symbol nm) } } if (p) - p->invoke(nm); + p->invoke(nm, no_next); else { skip_line(); return; @@ -3854,12 +3858,15 @@ static void decode_string_args(macro_iterator *mi) } } -void macro::invoke(symbol nm) +void macro::invoke(symbol nm, int no_next) { macro_iterator *mi = new macro_iterator(nm, *this); decode_args(mi); input_stack::push(mi); - tok.next(); + // we must delay tok.next() in case the function has been called by + // do_request to assure proper handling of compatible_flag + if (!no_next) + tok.next(); } macro *macro::to_macro() diff --git a/src/roff/troff/request.h b/src/roff/troff/request.h index 24d25890f..098de7cc1 100644 --- a/src/roff/troff/request.h +++ b/src/roff/troff/request.h @@ -1,5 +1,5 @@ // -*- C++ -*- -/* Copyright (C) 1989, 1990, 1991, 1992, 2000, 2001, 2002, 2004 +/* Copyright (C) 1989, 1990, 1991, 1992, 2000, 2001, 2002, 2004, 2008 Free Software Foundation, Inc. Written by James Clark ([email protected]) @@ -26,14 +26,14 @@ class macro; class request_or_macro : public object { public: request_or_macro(); - virtual void invoke(symbol s) = 0; + virtual void invoke(symbol, int) = 0; virtual macro *to_macro(); }; class request : public request_or_macro { REQUEST_FUNCP p; public: - void invoke(symbol); + void invoke(symbol, int); request(REQUEST_FUNCP); }; @@ -59,13 +59,13 @@ public: macro &operator=(const macro &); void append(unsigned char); void append(node *); - void append_unsigned(unsigned int i); - void append_int(int i); + void append_unsigned(unsigned int); + void append_int(int); void append_str(const char *); void set(unsigned char, int); unsigned char get(int); int length(); - void invoke(symbol); + void invoke(symbol, int); macro *to_macro(); void print_size(); int empty(); @@ -83,7 +83,7 @@ extern void init_node_requests(); extern void init_reg_requests(); extern void init_env_requests(); extern void init_hyphen_requests(); -extern void init_request(const char *s, REQUEST_FUNCP f); +extern void init_request(const char *, REQUEST_FUNCP); class charinfo; class environment; I think I want to refactor this to eliminate this `int no_next` function parameter. (It's now called `do_not_want_next_token`.) I have an idea for an alternative solution, already annotated in the source. $ sed -n 9539,9584p src/roff/troff/input.cpp // Consume the rest of the input line in copy mode and return it as a C // string; if, after spaces, the argument starts with a `"`, discard it, // letting any immediately subsequent spaces populate the returned // string. // // The caller must subsequently call `tok.next()`, _not_ `skip_line()`, // to advance the input stream pointer. // // TODO: Synthesize a newline token and push it onto the input stream so // that the foregoing becomes unnecessary, and request handlers can // uniformly use `skip_line()`. // // The caller has responsibility for `delete`ing the returned buffer. char *read_rest_of_line_as_argument() { int buf_size = 256; char *s = new char[buf_size]; // C++03: new char[buf_size](); (void) memset(s, 0, (buf_size * sizeof(char))); int c = read_character_in_copy_mode(0 /* nullptr */); while (' ' == c) c = read_character_in_copy_mode(0 /* nullptr */); if ('"' == c) c = read_character_in_copy_mode(0 /* nullptr */); int i = 0; while ((c != '\n') && (c != EOF)) { if (!is_invalid_input_char(c)) { if ((i + 2) > buf_size) { char *tem = s; s = new char[buf_size * 2]; // C++03: new char[buf_size * 2](); (void) memset(s, 0, (buf_size * 2 * sizeof(char))); memcpy(s, tem, buf_size); buf_size *= 2; delete[] tem; } s[i++] = c; } c = read_character_in_copy_mode(0 /* nullptr */); } s[i] = '\0'; if (0 == i) { delete[] s; return 0 /* nullptr */; } return s; } I furthermore think that implementing this newline synthesis + uniform `skip_line()` approach will fix bug #44714. I don't think the diagnosis there is correct--I don't think compatibility mode management is implicated at all. (We now handle compatibility mode with with a proper STL stack of `bool`, and doing so didn't fix that problem.) I look forward to attempting to prove my conjectures in the _groff_ 1.26 development cycle. An issue still to be dealt with is request handlers that read arguments in copy mode that _don't_ consume the rest of the input line. I need to see if there are any, and if so, they need to synthesize a newline and push it onto the token stream, but only if such an argument is the last thing on the input line. (They could be followed by ignored "extra" arguments, or a comment escape sequence.) _______________________________________________________ Reply to this item at: <https://savannah.gnu.org/bugs/?68558> _______________________________________________ Message sent via Savannah https://savannah.gnu.org/
signature.asc
Description: PGP signature
