Follow-up Comment #5, bug #68064 (group groff):

[comment #4 comment #4:]
> On the one hand, that suggests that whatever was going wrong in previous
> releases has been fixed.  On the other, that you and I get different results
> in 1.23 suggests there's an either intermittent or systems-specific bug at
> the root of it (not inconceivable, nroff being a shell script).  So I don't
> think we can be confident it's been fixed till we know what caused it.
> 
> In any case, you _have_ reproduced it in 1.23, which is where it was
> reported, so I'll reset the status from Unreproducible.  But I don't see that
> this should gate 1.24, so I'll set it to Postponed.

Well, for me, it was definitely a change in GNU _troff_ that fixed the
misbehavior, albeit not one I anticipated at the time.


$ git bisect start --term-old=old --term-new=new HEAD 1.23.0 --
src/roff/troff
...several iterations later...
$ make -C build -j troff groff
make: Entering directory '/home/branden/src/GIT/groff/build'
make: 'groff' is up to date.
  CXX      src/roff/troff/input.o
  CXXLD    troff
make: Leaving directory '/home/branden/src/GIT/groff/build'
$ printf '.sy /bin/false\n\\n[systat]\n'     |
GROFF_TEST_GROFF=build/test-groff ./build/nroff -U | cat -s
0
$ git bisect old
24e314b94c31c4b7b7908854455179cb97519001 is the first new commit
commit 24e314b94c31c4b7b7908854455179cb97519001
Author: G. Branden Robinson <[email protected]>
Date:   Tue Nov 12 09:08:22 2024 -0600

    [troff]: Slightly refactor (read_string() duties).
    
    Shift responsibility for calling `tok.next()` onto callers of
    `read_string()` instead of doing it internally.  For the purpose I have
    in mind (migrating `so` and `mso` to use `read_string()`), the latter
    advances the input stream pointer too early--better to let the caller
    control that.
    
    * src/roff/troff/input.cpp (read_string): Drop `tok.next()` call.
    
    * src/roff/troff/env.cpp (override_sizes): Add `tok.next()` call.

 ChangeLog                | 13 +++++++++++++
 src/roff/troff/env.cpp   |  1 +
 src/roff/troff/input.cpp |  3 ++-
 3 files changed, 16 insertions(+), 1 deletion(-)
$ git describe
1.23.0-2397-g88f003641
$ git checkout 24e314b94c31c4b7b7908854455179cb97519001
$ make -C build -j
...
$ printf '.sy /bin/false\n\\n[systat]\n'     |
GROFF_TEST_GROFF=build/test-groff ./build/nroff -U | cat -s
 256


Unfortunately, I don't think it'd be trivial to cherry-pick this commit onto a
_groff_ 1.23.0 tree; this commit was one piece of a major refactoring and
revision of input handling and, in full frankness, a change to GNU _troff_'s
input grammar as cautioned about in the "NEWS" for _groff_ 1.24.0.


*  GNU troff now strips a leading neutral double quote from the argument
   to the `cf`, `hpf`, `hpfa`, `lf`, `mso`, `msoquiet`, `nx`, `pi`,
   `pso`, `so`, `soquiet`, `sy`, and `trf` requests, and the second
   argument to the `open` and `opena` requests, allowing it to contain
   embedded leading spaces.


Worse, I can't account for how this revision corrected the bug reported in
comment #0.  Since my changes _were_ to unify syntax and reduce the amount of
per-request bespoke input handling, the hypothesis springs to mind is that an
unreported bug of long standing  was lurking in the `sy` request handler, and
I blithely blew that bug away when revising that handler to use common logic
with other requests (specifically, by calling `read_string()` instead of doing
its own lexical analysis of the input).

But that's just a guess.

And it doesn't look like a very promising one if we inspect that request
handler in the _groff_ 1.23.0 source tree....which, uhh, _already uses
`read_string()`...

https://cgit.git.savannah.gnu.org/cgit/groff.git/tree/src/roff/troff/input.cpp?h=1.23.0#n7658


void system_request()
{
  if (!unsafe_flag) {
    error("'sy' request is not allowed in safer mode");
    skip_line();
  }
  else {
    char *command = read_string();
    if (!command)
      error("empty command");
    else {
      system_status = system(command);
      delete[] command;
    }
  }
}


Here's the _groff_ 1.24.0.rc4 version.

https://cgit.git.savannah.gnu.org/cgit/groff.git/tree/src/roff/troff/input.cpp?h=1.24.0.rc4#n9430


void system_request()
{
  if (!has_arg(true /* peek */)) {
    warning(WARN_MISSING, "system command execution request expects a"
            " system command as argument");
    skip_line();
    return;
  }
  if (!want_unsafe_requests) {
    error("system command execution request is not allowed in safer"
          " mode");
    skip_line();
    return;
  }
  char *command = read_rest_of_line_as_argument();
  // `has_arg()` should have ensured that this pointer is non-null.
  assert(command != 0 /* nullptr */);
  if (0 /* nullptr */ == command)
    error("cannot apply system request to empty command");
  else
    system_status = system(command);
  delete[] command;
  tok.next();
}


So maybe I'm looking in the wrong place.


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?68064>

_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/

Attachment: signature.asc
Description: PGP signature

Reply via email to