bugGNU roff - Bugs: bug #65930, [me] large values of register `tv`...

 
 

bug #65930: [me] large values of register `tv` cause infinite trap recursion

Submitter:  Dave <barx>
Submitted:  Sat 29 Jun 2024 08:48:35 PM UTC
   
 
Category:  Macro package me Severity:  3 - Normal
Item Group:  Incorrect behaviour Status:  Confirmed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Open Planned Release:  None
* Mandatory Fields

Post a Comment

Add a New Comment Rich Markup
   

Discussion

Jump to the original submission

Tue 16 Jul 2024 01:15:42 PM UTC, comment #17: 

Okay, well, my patch from comment #2 seems to be impotent to resolve the problem reported in comment #0.


$ cat ATTIC/stack-limit.me
.mso e.tmac
.br
.nr $v 421
$ sed -n '/\.de @t/,+10p' tmac/e.tmac
.de @t                  \" --- output header or footer title
.if !\\n(?h \
\{\
.       \" Temporarily set vertical spacing to zero so `sz` doesn't
.       \" spring the footer trap again in infinite regress.
.       nr _t \\n(tv
.       nr tv 0
.       sz \\n(tp\"             \" set header/footer type fonts, etc.
.       @F \\n(tf
.       lt \\n(_Lu              \" make title span entire page
.       if \\n(?n \
$ ./build/test-groff -abww ATTIC/stack-limit.me
<beginning of page>
troff: backtrace: '/home/branden/src/GIT/groff/build/../tmac/e.tmac':1507: macro 'sz'
troff: backtrace: '/home/branden/src/GIT/groff/build/../tmac/e.tmac':470: macro '@t'
troff: backtrace: '/home/branden/src/GIT/groff/build/../tmac/e.tmac':458: macro '@e'
troff: backtrace: '/home/branden/src/GIT/groff/build/../tmac/e.tmac':405: trap-called macro '@f'
troff: fatal error: input stack limit exceeded (probable infinite loop)


Back to the drawing board, I reckon.

G. Branden Robinson <gbranden>
Group administrator
Tue 16 Jul 2024 01:10:40 PM UTC, comment #16: 

Interestingly, when I applied comment #5's patch to groff Git HEAD, I got some fuzz and offsets.


$ patch -p1 < me.65930.patch
patching file tmac/e.tmac
Hunk #1 succeeded at 463 with fuzz 1 (offset 4 lines).
Hunk #2 succeeded at 477 (offset 4 lines).


This recent commit accounts for the offset, I think:


commit 188471a29c9315ee45412403be95137f99d9177b
Author: Dave Kemper <saint.snit@gmail.com>
Date:   Sun May 14 02:38:15 2023 +0000

    [me]: Add debugging advice for developers.

    Fixes <https://savannah.gnu.org/bugs/?64189>.


The fuzz is more curious.

G. Branden Robinson <gbranden>
Group administrator
Tue 16 Jul 2024 01:05:18 PM UTC, comment #15: 

comment #14:

> comment #4:
> > since the `@t` internal macro is called by the `$h` and `$f`
> > "API" macros, it's the redefiner's responsibility to cope with
> > the more troublesome vertical spacing issues that arise.
>
> Agreed.  But for someone with existing -me code, who has already addressed the troublesome issues, I would hope a fix for this bug doesn't scuttle her carefully crafted solution.


I can't say for sure until I have a bug fix.

G. Branden Robinson <gbranden>
Group administrator
Wed 10 Jul 2024 09:46:33 PM UTC, comment #14: 

comment #4:

> since the `@t` internal macro is called by the `$h` and `$f`
> "API" macros, it's the redefiner's responsibility to cope with
> the more troublesome vertical spacing issues that arise.


Agreed.  But for someone with existing -me code, who has already addressed the troublesome issues, I would hope a fix for this bug doesn't scuttle her carefully crafted solution.  Having never created multi-line titles myself, I have neither a solid grasp of those issues, nor a good test for a regression.

> I therefore see no need to put the vanished verbiage back; the
> leading `$` in the macro names imply the needful.


Right, I was unclear here again: I was referring only to the example of what a user might want a redefined $h or $f for, addressed in the parenthetical "(e.g., multi-line)".  I agree the rest of the sentence is redundant with the general explanation of the leading $.

Dave <barx>
Group Member
Wed 10 Jul 2024 02:25:29 PM UTC, comment #13: 

This is waiting on info from me now.

G. Branden Robinson <gbranden>
Group administrator
Fri 05 Jul 2024 06:26:19 AM UTC, comment #12: 

It might also be worth your confirming that the patch file I attached (comment #5) matches the one you're applying, just to verify that I edited it correctly.

Dave <barx>
Group Member
Fri 05 Jul 2024 06:16:37 AM UTC, comment #11: 

Using the bug-65930b.me test file from comment #8, I can reproduce the problem in groff 1.22.4; using tv-stack-limit.me from comment #2, I cannot.

$ groff --version | head -n 1
GNU groff version 1.22.4
$ cat bug-65930b.me
.\"mso e.tmac
.br
.nr $v 421
.fo 'foo'bar'baz'
.lp
You get the diplomatic treatment
.br
You get the force-fed future
.br
You get the funk after death
.br
You get the weisenheimer brainstorm
$ groff -z -me ./bug-65930b.me
troff: fatal error: input stack limit exceeded (probable infinite loop)
$ groff -z -me ./tv-stack-limit.me
$ diff tv-stack-limit.me bug-65930b.me
1c1,3
< .nr $v 1640u
---
> .\"mso e.tmac
> .br
> .nr $v 421

As I noted in the original submission, for me "the bug only happens with the .br request present" (before the .nr, I should have clarified).  So the diff above, showing the absence of the triggering .br, makes me not surprised to see the respective results above.

But I am surprised that you're seeing the bug triggered in 1.22.4 with both files (with and without the initial .br).

With my recent inability to reproduce a different bug using seemingly the same input file and command invocation as the submitter (bug #65936), I have to wonder if I've Bjarnied my system somehow.  I checked the obvious things, like making sure I hadn't hacked on my 1.22.4 e.tmac file.  I do have various modifications to local font files, but I can't see how those matter, and indeed I get the same results even if I remove them.

So if you can confirm the bug shows up for you regardless of the presence/absence of the .br before the .nr, then we need to dig into what separates our 1.22.4s.  (Looking at an older release might seem like a wild goose chase, but since I'm also seeing different results from you using the patched -me on bleeding-edge groff, I think finding the difference between our systems in an official release might be easier than finding what could be different in independent new builds.)

Dave <barx>
Group Member
Fri 05 Jul 2024 05:52:24 AM UTC, comment #10: 

comment #9:

> These aren't exercising the formatter in the same way; they're
> using different output drivers


Yes, the intent was to show that I didn't see the bug with either the devps or devascii output driver.  Sorry I was unclear about that.

Dave <barx>
Group Member
Thu 04 Jul 2024 02:10:05 AM UTC, comment #9: 


$ groff -me tv-stack-limit.me | wc
    249     705    6268
$ groff -Tascii -me tv-stack-limit.me | uniq -c


These aren't exercising the formatter in the same way; they're using different output drivers (unless you have GROFF_TYPESETTER=ascii in your environment) with different vertical resolutions.

When using a `$v` of 421, I tested with the implicit 'ps' output device in every case and did reproduce the infinite recursion.

G. Branden Robinson <gbranden>
Group administrator
Thu 04 Jul 2024 02:07:22 AM UTC, comment #8: 

Hrm, I definitely can reproduce it, with a `$v` of 421, using groff 1.22.4.


$ /usr/bin/groff --version | head -n 1
GNU groff version 1.22.4
$ cat ./EXPERIMENTS/bug-65930b.me
.\"mso e.tmac
.br
.nr $v 421
.fo 'foo'bar'baz'
.lp
You get the diplomatic treatment
.br
You get the force-fed future
.br
You get the funk after death
.br
You get the weisenheimer brainstorm
$ /usr/bin/groff -z -me ./EXPERIMENTS/bug-65930b.me
troff: fatal error: input stack limit exceeded (probable infinite loop)


I thought briefly that the `-me` versus `mso` approaches of loading the package mattered, but they don't.


$ cat ./EXPERIMENTS/bug-65930c.me
.mso e.tmac
.br
.nr $v 421
.fo 'foo'bar'baz'
.lp
You get the diplomatic treatment
.br
You get the force-fed future
.br
You get the funk after death
.br
You get the weisenheimer brainstorm
$ /usr/bin/groff -z ./EXPERIMENTS/bug-65930c.me
troff: fatal error: input stack limit exceeded (probable infinite loop)


I can reproduce it with groff 1.23.0 and groff Git HEAD (my working copy, which doesn't have the patch applied) as well.

And if I go back to your original reproducer in comment #0, I can reproduce your problem with all the same versions of groff.

Can you reconfirm reproducibility of your original, then test each of my two variants?  Please test with 1.22.4, then 1.23.0, then HEAD.



G. Branden Robinson <gbranden>
Group administrator
Thu 04 Jul 2024 01:39:49 AM UTC, comment #7: 

OK.  So.

The patch has no effect on my original test file.  Test fails with it, fails without it.

Your updated test file (from comment #2) does not generate the error under any circumstances for me.  Not with patched -me, not with unpatched, not with a stock 1.22.4.

$ cat tv-stack-limit.me
.nr $v 1640u
.fo 'foo'bar'baz'
.lp
You get the diplomatic treatment
.br
You get the force-fed future
.br
You get the funk after death
.br
You get the weisenheimer brainstorm
$ groff --version | head -1
GNU groff version 1.22.4
$ groff -me tv-stack-limit.me | wc
    249     705    6268
$ groff -Tascii -me tv-stack-limit.me | uniq -c
     20
      1 You get the diplomatic treatment
     13
      1 You get the force-fed future
     13
      1 You get the funk after death
     13
      1 foo                          bar                         baz
     23
      1 You get the weisenheimer brainstorm
     41
      1 foo                          bar                         baz
      3


Dave <barx>
Group Member
Thu 04 Jul 2024 01:23:59 AM UTC, comment #6: 


comment #5:

> First speed bump: patch fails to apply.  This turns out to be because the tab characters in the patch have been reduced to strings of spaces.
> ... I now suspect there was a copy or paste translation error when pasting the patch into comment #2.


Urp, sorry--yes, I very likely did that instead of considerately attaching a patch file.

G. Branden Robinson <gbranden>
Group administrator
Thu 04 Jul 2024 01:19:07 AM UTC, comment #5: 

comment #2:

> Try this patch:


First speed bump: patch fails to apply.  This turns out to be because the tab characters in the patch have been reduced to strings of spaces.

I initially suspected being bitten by http://savannah.n ... g/support/?110582, but the workaround mentioned in that ticket still did not give me my tabs, so I now suspect there was a copy or paste translation error when pasting the patch into comment #2.

But I was able to fix the patch manually and get it to apply, so this speed bump was minor.  For posterity I'm attaching my patched patch here.

Now to actually run the patched -me code and see what happens!

(file #56226)

Dave <barx>
Group Member
Wed 03 Jul 2024 12:54:48 AM UTC, comment #4: 

comment #3:

> comment #2:
> > If I'm right about Allman not contemplating multi-line headers/footers
>
> His "-me Reference Manual" explicitly addressed such things when talking about the .$h (and, by reference, .$f) macro: "May be redefined to provide fancy (e.g., multi-line) headers."


Ah.  I had forgotten, and didn't check.

> Intriguingly, "someone" has removed this text from the groff version of the manual, perhaps in an attempt to hide his contemplation of such an abomination.


<laugh>  It's not so abominable from our end, nor from his I think (as we reused his code); since the `@t` internal macro is called by the `$h` and `$f` "API" macros, it's the redefiner's responsibility to cope with the more troublesome vertical spacing issues that arise.

I therefore see no need to put the vanished verbiage back; the leading `$` in the macro names imply the needful.  (See the second-to-last paragraph on page 1 of groff's meref.me.)

Or more simply:

"Go ahead and stick your fingers in, but be mindful of the teeth."

G. Branden Robinson <gbranden>
Group administrator
Wed 03 Jul 2024 12:02:36 AM UTC, comment #3: 

comment #2:

> Try this patch:


Thanks!  I'll give this a shot once I'm done chasing down a bash issue.

> If I'm right about Allman not contemplating multi-line headers/footers


His "-me Reference Manual" explicitly addressed such things when talking about the .$h (and, by reference, .$f) macro: "May be redefined to provide fancy (e.g., multi-line) headers."  Intriguingly, "someone" has removed this text from the groff version of the manual, perhaps in an attempt to hide his contemplation of such an abomination.

Dave <barx>
Group Member
Tue 02 Jul 2024 11:18:49 PM UTC, comment #2: 

Hi Dave,

Try this patch:


$ git diff
diff --git a/tmac/e.tmac b/tmac/e.tmac
index 48962de54..98b4761de 100644
--- a/tmac/e.tmac
+++ b/tmac/e.tmac
@@ -459,6 +459,10 @@ .de @e                     \" --- end page
 .de @t                 \" --- output header or footer title
 .if !\\n(?h \
 \{\
+.      \" Temporarily set vertical spacing to zero so `sz` doesn't
+.      \" spring the footer trap again in infinite regress.
+.      nr _t \\n(tv
+.      nr tv 0
 .      sz \\n(tp\"             \" set header/footer type fonts, etc.
 .      @F \\n(tf
 .      lt \\n(_Lu              \" make title span entire page
@@ -469,6 +473,9 @@ .de @t                      \" --- output header or footer title
 .      br
 .      if \\n(?n \
 .              nm
+.      nr tv \\n(_t
+.      rr _t
+.      sz \\n(tp\"             \" restore vertical spacing
 .\}
 ..
 .


I used the following very slightly more realistic document to further test it.


$ cat EXPERIMENTS/tv-stack-limit.me
.nr $v 1640u
.fo 'foo'bar'baz'
.lp
You get the diplomatic treatment
.br
You get the force-fed future
.br
You get the funk after death
.br
You get the weisenheimer brainstorm


Two caveats:

1.  With a well-tuned `tv` (or `$v`), you can cause the footer to overprint body text (the above does in nroff mode but not troff mode).  I haven't explored whether this is possible with less huge but still crafty values on older _groff_s.

2.  The bottom margin grows huge in this example.  I don't know yet if this is a facet of bug #60553, but it may well be.  If so, this patch suggests a similar solution to that ticket: compute the vertical margins based on a `tv` of zero.  If I'm right about Allman not contemplating multi-line headers/footers (a restriction that is true of ms but not mm), then that approach may resolve that issue.

Do you think this patch is good enough as-is to resolve this ticket?

G. Branden Robinson <gbranden>
Group administrator
Tue 02 Jul 2024 10:24:05 PM UTC, comment #1: 

Hi Dave,

The `@f` trap macro is setting itself off recursively.  This is not quite obvious with `groff -b` but is if one instruments the `@t` macro.


$ git diff
diff --git a/tmac/e.tmac b/tmac/e.tmac
index 48962de54..7453f3e39 100644
--- a/tmac/e.tmac
+++ b/tmac/e.tmac
@@ -459,6 +459,7 @@ .de @e                      \" --- end page
 .de @t                 \" --- output header or footer title
 .if !\\n(?h \
 \{\
+.      do tm GBR: \\$0: \\$@
 .      sz \\n(tp\"             \" set header/footer type fonts, etc.
 .      @F \\n(tf
 .      lt \\n(_Lu              \" make title span entire page
$ ./build/test-groff -bz -me EXPERIMENTS/tv-stack-limit.me 2>&1|head -n 20
GBR: @t: "$h"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"
GBR: @t: "$f"


Maybe we want to validate `tv` (and `dv`) values and enforce a limit...

...or maybe we should turn vertical position traps off while setting headers and footers.  I wonder if Allman contemplated titles taller than one vee; if not, vertical spacing is irrelevant in that context.

...so maybe what we want to do is temporarily set vertical spacing to zero inside headers and footers.

G. Branden Robinson <gbranden>
Group administrator
Sat 29 Jun 2024 08:48:35 PM UTC, original submission:  

This bug is in at least 1.22.4 and 1.23.  The below example uses the register name "$v" rather than its more modern alias "tv", so that the example works the same in current and pre-1.23 code (which had not yet defined "tv").

$ cat stack-limit.me
.mso e.tmac
.br
.nr $v 421
$ groff stack-limit.me | wc
troff: fatal error: input stack limit exceeded (probable infinite loop)
    238     780    4673

Observations:

  • The bug only happens with the .br request present.
  • Values 420 and lower do not trigger the bug.  Values 421 and higher do.
    • This applies to output on the ps device.  On the ascii device, 421 does not trigger the bug, but 440 does.
  • If any text is appended to the input file (either with or without a -me paragraphing macro), that text is present in the output, despite the error above it being "fatal."


Yes, 421 is higher than most real-world values for the $v/tv register, so I don't have a problem with this exceeding a supported limit.  But exceeding that limit shouldn't provoke an ostensibly fatal error.  And "stack limit exceeded" is a peculiar error for this to trigger.

Dave <barx>
Group Member

 

Attached Files

Attached Files
file #56226:  me.65930.patch added by barx (656B - text/x-diff - comment #2 patch with corrected tab characters)

(Note: upload size limit is set to 4.0MiB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

 

Dependencies

This item does not depend on any other items.

No items depend on this one.

 

Mail Notification Carbon-Copy List

Carbon-Copy List
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by barx (Submitted the item)
  •  

    Votes

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

    History

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-07-16 gbranden StatusNeed Info Confirmed
    2024-07-10 gbranden Assigned tobarx gbranden
    2024-07-04 barx Attached File- Added me.65930.patch, #56226
    2024-07-02 gbranden StatusConfirmed Need Info
        Assigned toNone barx
        Summary[me] Large values of register &quot;tv&quot; cause fatal error [me] large values of register `tv` cause infinite trap recursion
    2024-07-02 gbranden StatusNone Confirmed

    Back to the top

    Powered by Savane 3.16-a7ba.
    Corresponding source code