bugGNU roff - Bugs: bug #61089, [troff] assertion failed:...

 
 

bug #61089: [troff] assertion failed: 'desired_space.is_zero() && nspaces == 0'

Submitter:  John Gardner <alhadis>
Submitted:  Mon 30 Aug 2021 02:57:35 AM UTC
   
 
Category:  Core Severity:  4 - Important
Item Group:  Crash/Unresponsive Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 04 Sep 2021 04:58:29 PM UTC, comment #16: 

Step #3 is filed as bug #61117.

G. Branden Robinson <gbranden>
Group administrator
Sat 04 Sep 2021 04:53:34 PM UTC, comment #15: 

Step #2, using Heirloom Doctools semantics for a zero output line length, is filed as bug #61116.

G. Branden Robinson <gbranden>
Group administrator
Sat 04 Sep 2021 04:43:30 PM UTC, comment #14: 

The assertion failure is fixed.  The remaining items I will tackle in separate tickets.


commit 69efbe0a69a8e7de8904d78e3de8c7e8a58a8b92
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Sat Sep 4 23:20:54 2021 +1000

    [troff]: Don't adjust nonadjustable lines.

    This means that the direction from which an output line in adjustment
    mode "b" (or its "n" synonym) is filled with supplemental space is not
    changed if that output line does not require adjustment.  This will
    result in whitespace changes to documents using that adjustment mode,
    and these changes will be plainly visible on low-resolution output
    devices like terminals.

    To illustrate, in the following "A" means an output line requiring
    adjustment; "F" a line that is "full" and does not; and "L" and "R"
    indicate distribution of adjustment spaces from the left and right,
    respectively.

    groff 1.22.4    groff 1.23.0
    ------------    ------------
    A    L          A    L
    A    R          A    R
    F    L          F    R
    A    R          A    L

    * src/roff/troff/env.cpp (distribute_space): Return early if either the
      amount of desired space to be distributed or the count of space nodes
      in the output line to distribute it among is zero.

    * tmac/tests/an_TH-repairs-ad-damage.sh: Update test to expect space to
      be distributed differently.

    Fixes <https://savannah.gnu.org/bugs/?61089> and
    <https://savannah.gnu.org/bugs/index.php?60673>.


G. Branden Robinson <gbranden>
Group administrator
Sat 04 Sep 2021 10:51:58 AM UTC, comment #13: 

This experimental patch was instructive.


diff --git a/src/roff/troff/env.cpp b/src/roff/troff/env.cpp
index 4af8741e1..1b6a15755 100644
--- a/src/roff/troff/env.cpp
+++ b/src/roff/troff/env.cpp
@@ -2053,14 +2053,15 @@ static node *node_list_reverse(node *n)
 static void distribute_space(node *n, int nspaces, hunits desired_space,
                             int force_reverse = 0)
 {
+  hunits em = curenv->get_size();
+  double Ems = (double)desired_space.to_units() / nspaces
+              / (em.is_zero() ? hresolution : em.to_units());
+  debug("spreading %1m per space among %2 spaces", Ems, nspaces);
   static int reverse = 0;
   if (force_reverse || reverse)
     n = node_list_reverse(n);
   if (!force_reverse && nspaces > 0 && spread_limit >= 0
       && desired_space.to_units() > 0) {
-    hunits em = curenv->get_size();
-    double Ems = (double)desired_space.to_units() / nspaces
-                / (em.is_zero() ? hresolution : em.to_units());
     if (Ems > spread_limit)
       output_warning(WARN_BREAK, "spreading %1m per space", Ems);
   }


Input (pretty much John's reduced reproducer):


$ cat EXPERIMENTS/61089.roff
.de _
.       na
.       nh
.       ll 0
.       di A
.tm line length in diversion is \\n(.l
\&\\$1
.       di
.       br
..
._ " XYZ"
.tm line length is \n(.l
.A


Output:


line length in diversion is 0
troff: backtrace: 'EXPERIMENTS/61089.roff':7: macro '_'
troff: backtrace: file 'EXPERIMENTS/61089.roff':11
troff: EXPERIMENTS/61089.roff:11: debug: spreading -nanm per space among 0 spaces
troff: backtrace: 'EXPERIMENTS/61089.roff':7: macro '_'
troff: backtrace: file 'EXPERIMENTS/61089.roff':11
troff: EXPERIMENTS/61089.roff:11: warning [p 0, 0.0i, div 'A', 0.0i]: line has non-positive width 0m
line length is 0
troff: warning [p 1, 0.0i]: can't break line
troff: debug: spreading 0m per space among 1 spaces
troff: ../src/roff/troff/env.cpp:2074: distribute_space(): assertion failed: 'desired_space.is_zero() && nspaces == 0'
/home/branden/src/GIT/groff/build/groff: error: troff: Aborted (core dumped)


Yeaaaaah.  Check that out.  NaNs (division by zero) and an attempt to spread 0m of space.

I think resolving this mess will involve three steps.

(1) Make distribute_space() more irritable about its inputs.  If either `nspaces` or `desired_space` is zero, return early.  I might instrument this check with a diagnostic, too, but maybe not because...

(2) In the short run I think we should do as Heirloom Doctools troff does and clamp the line length to the horizontal resolution.  This will require checks on the implementation of the `ll` request and the code that reads the DESC file in the first place.  I'm reasonably familiar with these areas of the codebase.

(3) Bring John's proposed zero-line-length semantics to the groff mailing list.  I have no particular objection to it but since that list is effectively the entire troff community's sounding board, I think it's prudent to notify and seek the feedback of other implementors and experts.  If it survives scrutiny there, implement it.

Whether (3) survives or not, I think either (1) or (2) should make it unnecessary to add checks at the call sites of `distribute_space()`.  (And if I'm wrong, there is only a handful of such places.)

G. Branden Robinson <gbranden>
Group administrator
Sat 04 Sep 2021 03:14:47 AM UTC, comment #12: 


> What do you expect the semantics of an output line length of zero to be?


Like this:


This is your line-length

This
is
your
line-
length
on
drugs


Basically, print as many characters as you can fit up until the next break opportunity (which could be whitespace or a hyphenation opportunity, depending on hyphenation settings).

John Gardner <alhadis>
Fri 03 Sep 2021 12:50:59 PM UTC, comment #11: 

comment #9:

> > John, is this enough information for you to put together a (much) smaller reproducer?
>
> Aye, I narrowed it down to the following test-case:
>


> 01 .de _
> 02 .        na
> 03 .        nh
> 04 .        ll 0
> 05 .        di A
> 06 \&\\$1
> 07 .        di
> 08 .        br
> 09 ..
> 10 ._ " XYZ"


>
> Removing \& from line 6 causes the error to disappear, as does passing "XYZ" without a leading space; i.e.,


> ._ "XYZ"


> but not


> ._ " XYZ"


>


Thanks, John.

I find myself troubled by a more fundamental issue.

What do you expect the semantics of an output line length of zero to be?  CSTR #54 does not seem to address this issue, and Heirloom Doctools troff appears to clamp the line length to the minimum horizontal device resolution (which seems reasonable) (see below).

Not saying there's no bug here, but maybe if we did as Heirloom does (and presumably Version 7 Unix troff did--I can check), this assertion would never trip.


$ cat ll.roff
.tm horizontal resolution .H is \n(.Hu
.nr ll 49u
.de m1
.       ll \\n(llu
.       tm ll: \\n(llu; .l: \\n(.lu
.       nr ll -1
.       if \\n(llu>=0 .m1
..
.m1
$ ./bin/nroff ./ll.roff
horizontal resolution .H is 24u
ll: 49u; .l: 48u
ll: 48u; .l: 48u
ll: 47u; .l: 48u
ll: 46u; .l: 48u
ll: 45u; .l: 48u
ll: 44u; .l: 48u
ll: 43u; .l: 48u
ll: 42u; .l: 48u
ll: 41u; .l: 48u
ll: 40u; .l: 48u
ll: 39u; .l: 48u
ll: 38u; .l: 48u
ll: 37u; .l: 48u
ll: 36u; .l: 24u
ll: 35u; .l: 24u
ll: 34u; .l: 24u
ll: 33u; .l: 24u
ll: 32u; .l: 24u
ll: 31u; .l: 24u
ll: 30u; .l: 24u
ll: 29u; .l: 24u
ll: 28u; .l: 24u
ll: 27u; .l: 24u
ll: 26u; .l: 24u
ll: 25u; .l: 24u
ll: 24u; .l: 24u
ll: 23u; .l: 24u
ll: 22u; .l: 24u
ll: 21u; .l: 24u
ll: 20u; .l: 24u
ll: 19u; .l: 24u
ll: 18u; .l: 24u
ll: 17u; .l: 24u
ll: 16u; .l: 24u
ll: 15u; .l: 24u
ll: 14u; .l: 24u
ll: 13u; .l: 24u
ll: 12u; .l: 24u
ll: 11u; .l: 24u
ll: 10u; .l: 24u
ll: 9u; .l: 24u
ll: 8u; .l: 24u
ll: 7u; .l: 24u
ll: 6u; .l: 24u
ll: 5u; .l: 24u
ll: 4u; .l: 24u
ll: 3u; .l: 24u
ll: 2u; .l: 24u
ll: 1u; .l: 24u
ll: 0u; .l: 24u


G. Branden Robinson <gbranden>
Group administrator
Thu 02 Sep 2021 08:58:30 PM UTC, comment #10: 

comment #8:

> Reverting this one change definitely restores groff 1.22.4
> behavior for John's ono.tmac...


That is some kind of voodoo.  There should be only two possibilities:

  1. output_width > 0: in this case, the new "if" is false, and the code drops through to where it would have without the new block; i.e. there no functional change.
  2. output_width <= 0: this activates the new code path, which emits a warning, then exits the function where it didn't before, which could have any number of subsequent effects.


I don't see how the behavior changes but no warning is emitted.  Is output_warning() failing somehow?

Dave <barx>
Group Member
Thu 02 Sep 2021 07:51:16 PM UTC, comment #9: 


> John, is this enough information for you to put together a (much) smaller reproducer?


Aye, I narrowed it down to the following test-case:


01 .de _
02 .        na
03 .        nh
04 .        ll 0
05 .        di A
06 \&\\$1
07 .        di
08 .        br
09 ..
10 ._ " XYZ"


Removing \& from line 6 causes the error to disappear, as does passing "XYZ" without a leading space; i.e.,

._ "XYZ"

but not

._ " XYZ"


John Gardner <alhadis>
Thu 02 Sep 2021 01:20:44 PM UTC, comment #8: 

Returning to the groff side of things, and why my infinite-loop-fixing commit caused this problem, here is the code change in question.  (There were no other changes in that commit except to the ChangeLog and to add editor aid comments to env.cpp.)


diff --git a/src/roff/troff/env.cpp b/src/roff/troff/env.cpp
index 9cbc13048..cb6b1b1af 100644
--- a/src/roff/troff/env.cpp
+++ b/src/roff/troff/env.cpp
@@ -2142,6 +2142,14 @@ void environment::possibly_break_line(int start_here, int forced)
     }
     distribute_space(pre, bp->nspaces, extra_space_width);
     hunits output_width = bp->width + extra_space_width;
+    // This should become an assert() when we can get reliable width
+    // data from CJK glyphs.  See Savannah #44018.
+    if (output_width <= 0) {
+      double output_width_in_ems = output_width.to_units();
+      output_warning(WARN_BREAK, "line has non-positive width %1m",
+                    output_width_in_ems);
+      return;
+    }
     input_line_start -= output_width;
     if (bp->hyphenated)
       hyphen_line_count++;


Reverting this one change definitely restores groff 1.22.4 behavior for John's ono.tmac...and definitely resurrects the infinite loop that the change corrected, since I didn't revert the regression test for it.

Bummer.

Well, as the guru says, the road to enlightenment is paved with HTFs and WTFs.

G. Branden Robinson <gbranden>
Group administrator
Thu 02 Sep 2021 01:10:21 PM UTC, comment #7: 

Getting to John's actual code, the problem appears to be the emission of an empty line while inside a diversion, but only sometimes.

The trouble occurs in a string-downcasing macro (which uses the new groff .stringdown request if it is available).

I added crude instrumentation to the macro file.  It is at present a mystery to me why outputting an empty line inside a diversion is causing the assertion failure in one case, but not the other.

John, is this enough information for you to put together a (much) smaller reproducer?


603 .de ..
604 .       tm GBR entering macro ..
605 .       ie ''\\n(.z' .tm GBR we're not in a diversion
606 .       el           .tm GBR we're in diversion '\\n(.z'
607 .       \" Use Groff's .stringdown request if available
608 .       if d stringdown \{\
609 .               tm GBR using groff .stringdown
610 .               if '\&'\\$1' .if d \\$2 \{\
611 .                       tmc GBR first arg was an input break and second
612 .                       tm arg is defined
613 .                       stringdown \\$2
614 .                       return
615 .               \}
616 .               tm GBR string to be downcased is '\\*(!^B'
617 .               ds !^B \\$*
618 .               stringdown !^B
619 .               tm GBR downcase successful; emitting string (BOMBS HERE)
620 \&\\*(!^B
621 .               tm GBR done emitting string
622 .               rm !^B
623 .               return
624 .       \}
625 .       tm GBR done with groff conditional in macro ..
626 .
627 .       \" Modify by reference


Here's the tail of the output the foregoing produces.


troff: t/6.2-string-slugs.roff:3: warning: macro '>>' not defined
GBR calling macro .. to downcase string
GBR entering macro ..
GBR we're in diversion '~'
GBR using groff .stringdown
GBR string to be downcased is ''
GBR downcase successful; emitting string (BOMBS HERE)
GBR done emitting string
GBR calling macro .. to downcase string
GBR entering macro ..
GBR we're in diversion '~'
GBR using groff .stringdown
GBR string to be downcased is ''
GBR downcase successful; emitting string (BOMBS HERE)
troff: ../src/roff/troff/env.cpp:2073: distribute_space(): assertion failed: 'desired_space.is_zero() && nspaces == 0'
/home/branden/src/GIT/groff/build/groff: error: troff: Aborted (core dumped)


G. Branden Robinson <gbranden>
Group administrator
Thu 02 Sep 2021 11:49:33 AM UTC, comment #6: 


comment #4:

> comment #3:
> > There are reasons to believe it isn't the culprit though:
>
> I'd add (3) the newly introduced code path emits a warning, so we can tell if the code is following commit bcdf2f4c's path by whether the new warning occurs.  And even with all warnings enabled, t/6.2-string-slugs.roff trips no new warnings in 1.23.0.rc1.
>
> (Running this code with all warnings on is still instructive, though, as it hints at syntax errors in ono.tmac, which emits several "unbalanced .el request" warnings.)


I feel both vindicated and self-pwn3d.

A git bisect reveals that the problematic commit is exactly the one I singled out.

I will try to figure out WTF.

G. Branden Robinson <gbranden>
Group administrator
Thu 02 Sep 2021 04:37:31 AM UTC, comment #5: 

comment #2:

> Is there anything I can do to pinpoint it?


As Branden says, a bisect to pinpoint the problematic commit is the next logical step.  I hesitate to do that myself because building groff is painfully slow on my ancient processor.  But if you have some basic familiarity with git and don't mind building groff multiple times, even if you've never done a bisect before, the git-bisect man page walks you through the process pretty clearly.

Dave <barx>
Group Member
Thu 02 Sep 2021 04:10:52 AM UTC, comment #4: 

comment #3:

> There are reasons to believe it isn't the culprit though:


I'd add (3) the newly introduced code path emits a warning, so we can tell if the code is following commit bcdf2f4c's path by whether the new warning occurs.  And even with all warnings enabled, t/6.2-string-slugs.roff trips no new warnings in 1.23.0.rc1.

(Running this code with all warnings on is still instructive, though, as it hints at syntax errors in ono.tmac, which emits several "unbalanced .el request" warnings.)

Dave <barx>
Group Member
Wed 01 Sep 2021 08:21:22 AM UTC, comment #3: 

There is a vague possibility that this is the change that uncovered the latent bug--this is the only change to adjustment code that I can recall since I've been involved with groff.

There are reasons to believe it isn't the culprit though: (1) while the change is to a function that calls `distribute_space()`, the change is after that call, not before it and (2) the nature of my change is to issue a diagnostic and bewail the fact that we can't make an assertion, rather than hazardously larding `distribute_space()` with invalid preconditions.

I'll try to repro the problem and bisect.


commit bcdf2f4c7c28328c711c6a7ac2ea17f2ecd5cdd4
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Wed Oct 21 00:29:24 2020 +1100

    src/roff/troff/env.cpp: Avoid infinite loop.

    * src/roff/troff/env.cpp (environment::possibly_break_line): Emit break
      warning and return if the output width is not positive.  The code
      assumes that it will be and loops infinitely if it isn't.  I _think_
      this is because we're not able to get width data for (some?) CJK
      glyphs.  Based on a patch by Osamu Sayama.

    Fixes <https://savannah.gnu.org/bugs/index.php?44018>.


G. Branden Robinson <gbranden>
Group administrator
Wed 01 Sep 2021 07:49:32 AM UTC, comment #2: 


> So the change that introduced this assertion failure is somewhere further up the call chain.


Is there anything I can do to pinpoint it?

John Gardner <alhadis>
Tue 31 Aug 2021 11:48:03 AM UTC, comment #1: 

original submission:

> a regression was introduced by some recent changes.


...but not too recent, since the problem also occurs in 1.23.0.rc1.

distribute_space(), the function in which the assertion occurs, has not changed since 2002.  So the change that introduced this assertion failure is somewhere further up the call chain.

Dave <barx>
Group Member
Mon 30 Aug 2021 02:57:35 AM UTC, original submission:  

Using a fresh build of Groff (checked out at a1647c997d8ecbbf4bb58b934a71eff7c794c52e), I encountered this error:


troff: src/roff/troff/env.cpp:2073: distribute_space(): assertion failed: 'desired_space.is_zero() && nspaces == 0'
groff: error: troff: Abort trap: 6
grotty:<standard input>:14: warning: no final 'x stop' command


To reproduce:


git clone https://github.com/Alhadis/Mono && cd Mono
git checkout 89aea0a586266405d23def958412f35943e313f3
groff -Tutf8 t/6.2-string-slugs.roff


This doesn't happen with Groff v1.22.4, which indicates that a regression was introduced by some recent changes.

John Gardner <alhadis>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-09-04 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2021-09-04 gbranden StatusNeed Info In Progress
    2021-09-03 gbranden StatusConfirmed Need Info
    2021-09-01 gbranden StatusNone Confirmed
        Assigned toNone gbranden
        Summarydistribute_space(): assertion failed: [troff] assertion failed: 'desired_space.is_zero() && nspaces == 0'
    2021-08-31 barx Severity3 - Normal 4 - Important

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code