bugGNU troff - Bugs: bug #44018, [PATCH] gtroff hangs in...

 
 

bug #44018: [PATCH] gtroff hangs in environment::possibly_break_line with -mja

Submitted by:  Osamu Sayama <sayama>
Submitted on:  Fri 16 Jan 2015 04:51: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)
   

Comment Type & Canned Response:
       No canned response available

 

Sat 25 Sep 2021 02:35:14 PM UTC, comment #5: 

Looking at that code, i conclude that is already coded very defensively.  What "int width = 24; int w = wcwidth(get_code(g)); if (w > 1) width *= w;" does is this:

  • If and only if the C library is sure that the character is double width, reserve two columns for it.
  • In all other cases, reserve one column for it.  That includes normal single-width characters, normal zero width-characters (even though you say they will not usually show up here, but groff allows the user to input any character and the C library does not provide an API to find out whether a character is combining, the only indication it provides is wcwidth=0, so short of filtering out wcwidth=0 characters earlier, groff cannot prevent them from showing up here), and it includes non-printable characters and invalid code points.

Always reserving a width of at least 24 no matter how broken libc might be is clearly safe and cannot result in a total width of zero, so i fear we are looking in the wrong place and this hang is not related to wcwidth, at least neither to this call of wcwidth nor to the other call in this file, which uses the same logic.

I only realize now that this was already fixed in bcdf2f4c7c28328c711c6a7ac2ea17f2ecd5cdd4 (Oct 21 00:29:24 2020 +1100).  But given the present analysis, isn't the commit message and code comment in that commit misleading?

Ingo Schwarze <schwarze>
Project Member
Sat 25 Sep 2021 09:08:50 AM UTC, comment #4: 

comment #3:

> Re comment #2:
>
> I sense a possible contradiction in this comment: "returning zero" is not the same as "negative character widths".


Indeed not, and I was not trying to imply that it did.

Perhaps it would be more helpful for me to share a piece of stashed git change I have, particularly since I need to update this ticket anyway, with the finding that a negative value from `wcwidth()` is returned via `font::get_width()`, sometimes, but only if the later is called through the output driver--I can't get troff itself to provoke this behavior, even by requesting the offending glyph's width explicitly with the `\w` escape.

The problem crops up if you add an `assert(w > 0)` and then try to build `pic.html`.  The instrumentation might look a little silly--it's to keep the terminal from getting blitzed: `font::get_width()` gets called a lot.

diff --git a/src/libs/libgroff/font.cpp b/src/libs/libgroff/font.cpp
index 9f578478e..204d86dae 100644
--- a/src/libs/libgroff/font.cpp
+++ b/src/libs/libgroff/font.cpp
@@ -377,11 +377,22 @@ int font::get_width(glyph *g, int point_size)
     return w;
   }
   if (is_unicode) {
-    // Unicode font
+    // The output device uses Unicode encoding.
     int width = 24; // XXX: Add a request to override this.
     int w = wcwidth(get_code(g));
-    if (w > 1)
-      width *= w;
+    assert(w != 0);
+    // XXX: POSIX says that wcwidth() should return -1 if its argument
+    // "does not correspond to a printable wide-character code", but
+    // glibc ~2.28 returns it for printable codes that Unicode Annex #11
+    // calls "ambiguous" in the property "East Asian width".  UAX #11
+    // says that the width is then context-dependent; only our caller
+    // knows this context.  In the future we may need to stop taking the
+    // absolute value and expect the caller to handle a negative width.
+    if (get_code(g) == 8220)
+      debug("GBR: getting width of glyph 8220\n");
+    if (w < 0)
+      debug("GBR: width of glyph %1 is %2\n", get_code(g), w);
+    width *= abs(w);
     if (real_size == unitwidth || font::use_unscaled_charwidths)
       return width;
     else

 

> Regarding the first sentence, for wcwidth(3), a zero return value means the character takes up no horizontal space.  That is the expected return value for many characters for a variety of reasons, for example zero-width spaces and combining accents.  So i expect it would be possible to construct a line of total length zero by including several character, but all of them of wcwidth zero.


That's not what seems to happen in practice for groff; combining characters have either been filtered out or have glyph definitions that assign them positive width.  This makes sense to me--output drivers in general aren't character cell terminals, and it's helpful to know the width of a combining glyph so that you can center it with respect to the glyph it's combining with, for instance.

> Regarding the second sentence, for wcwidth(3), a return value of -1 means that the character is not printable.


glibc 2.28 seems to have its own ideas.  :-/

> In that case, adding that -1 to the line length would indeed be a bug.  Possible courses of actions are erroring out, inserting a replacement character, or just skipping the character, whatever seems most helpful in the case at hand.


Bear in mind here that the value returned by `wcwidth()` is used as a multiplier.  The branch we're in in `font::get_width()` says `if (is_unicode)`, which restricts us to the grotty and grohtml drivers.

(I know you have a strong distaste for grohtml--can we take that as read?)

> Since wcwidth(3) is usually provided by the operating system in the C library, portable code must make sure that it can deal with any return value for any character.  Subtle differences exist between the wcwidth(3) return values of some characters between Linux and BSD and even between different BSD systems.  In some commercial systems, most notably even the latest releases of Oracle Solaris, wcwidth(3) is quite badly broken and returns clearly wrong values for many characters, including -1 or 0 for many characters that should be width +1 or +2.  On such systems, portable software cannot reasonably avoid minor output glitches, but it can implement careful and correct handling of return values to reduce the risk of letting wrong return values go unnoticed, and even more so to avoid hangs and crashes.


Agreed.  I'm trying to get `font::get_width()` into a place where it is coded more defensively.

Fixing the problem with `pic.html` generation might be as simple as adding a glyph definition to devhtml/font/R.proto for it--both `lq` and `rq` are missing.

There will of course be other glyphs for which glibc's `wcwidth()` returns -1, perhaps wrongly.  I don't know yet if it will be better to just patch around that with glyph definitions on the driver side, or do something else.

G. Branden Robinson <gbranden>
Project Administrator
Fri 24 Sep 2021 12:03:56 PM UTC, comment #3: 

Re comment #2:

I sense a possible contradiction in this comment: "returning zero" is not the same as "negative character widths".

Regarding the first sentence, for wcwidth(3), a zero return value means the character takes up no horizontal space.  That is the expected return value for many characters for a variety of reasons, for example zero-width spaces and combining accents.  So i expect it would be possible to construct a line of total length zero by including several character, but all of them of wcwidth zero.

Regarding the second sentence, for wcwidth(3), a return value of -1 means that the character is not printable.  In that case, adding that -1 to the line length would indeed be a bug.  Possible courses of actions are erroring out, inserting a replacement character, or just skipping the character, whatever seems most helpful in the case at hand.

Since wcwidth(3) is usually provided by the operating system in the C library, portable code must make sure that it can deal with any return value for any character.  Subtle differences exist between the wcwidth(3) return values of some characters between Linux and BSD and even between different BSD systems.  In some commercial systems, most notably even the latest releases of Oracle Solaris, wcwidth(3) is quite badly broken and returns clearly wrong values for many characters, including -1 or 0 for many characters that should be width +1 or +2.  On such systems, portable software cannot reasonably avoid minor output glitches, but it can implement careful and correct handling of return values to reduce the risk of letting wrong return values go unnoticed, and even more so to avoid hangs and crashes.

Ingo Schwarze <schwarze>
Project Member
Fri 24 Sep 2021 08:07:04 AM UTC, comment #2: 

I've been spending some time in libgroff's font.cpp lately and I've ruled out wcwidth() returning zero for character codes (which is a really good thing--that would have broken other assumptions in groff).

So wherever the formatter is getting this notion of the line length being zero, it's not by summing negative character widths.

G. Branden Robinson <gbranden>
Project Administrator
Wed 21 Oct 2020 07:07:50 AM UTC, comment #1: 

If we had better CJK support, this would've been a severity 5 - blocker.  Pretty bad to just hang forever.

Also pretty bad to take almost 6 years to fix an infinite loop. :-/

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>.

commit 7964eb52debd3f09ce9125ca063c742448854fa8
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Wed Oct 21 00:39:39 2020 +1100

    Add regression test for Savannah #44018.

    Warning: "make check" will loop infinitely at this commit.

G. Branden Robinson <gbranden>
Project Administrator
Fri 16 Jan 2015 04:51:35 AM UTC, original submission:  

I encountered that gtroff hangs with some Japanese roff file
if -mja macro is used. I'm using groff-1.22.2 with ja_JP.UTF-8
locale. I could reproduce this problem in some of destro like Ubuntu14.04
which includes groff-1.22.2. For example, by using attached sample1.1,
---
% preconv -D utf8 sample1.1 | gtbl | geqn -Tutf8 | gtroff -E -Tutf8 -mandoc -mja | grotty -c
---
It hangs. From pstack, it goes into an infinite loop at
environment::possibly_break_line.
---
$ pstack `pgrep troff`
#0  0x000000000040f532 in environment::possibly_break_line(int, int) ()
#1  0x000000000040f950 in environment::space_newline() ()
#2  0x0000000000410a30 in environment::newline() ()
#3  0x0000000000422594 in process_input_stack() ()
#4  0x0000000000422d83 in process_input_file(char const*) ()
#5  0x0000000000402e1b in main ()
---
Attached sample2_TP.1 is another example for this. I think
this is a bug in nroff file itself because .TP must have 2
arguments, first one is label, second one is its explanation.
However, I think gtroff should avoid hang.

From my simple investigation, output_width becomes 0 when this problem occurs.

     hunits output_width = bp->width + extra_space_width;

So returning this function if output_width <= 0 resolved my issue.

--- groff-1.22.2/src/roff/troff/env.cpp.org     2013-02-07 21:06:06.000000000 +0
900
+++ groff-1.22.2/src/roff/troff/env.cpp 2015-01-14 10:23:07.817142586 +0900
@@ -2110,6 +2110,9 @@
     }
     distribute_space(pre, bp->nspaces, extra_space_width);
     hunits output_width = bp->width + extra_space_width;
+    if (output_width <= 0)
+      return;
+
     input_line_start -= output_width;
     if (bp->hyphenated)
       hyphen_line_count++;


I'm not sure about the root cause of this 0 width (bp->width and extra_space_width
were 0) however I think it's great that going into a infinite loop is avoided
even if output_width is 0.

Osamu Sayama <sayama>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #32837:  sample1.1 added by sayama (117B - application/octet-stream)
file #32838:  sample2_TP.1 added by sayama (576B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by schwarze (Posted a comment)
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by bgarrigues (Updated the item)
  • -email is unavailable- added by sayama (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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-21 gbranden Severity3 - Normal => 4 - Important
        StatusNone => Fixed
        Assigned toNone => gbranden
        Open/ClosedOpen => Closed
        Planned ReleaseNone => 1.23.0
    2020-10-13 barx Carbon-CopyRemoved 93119 => -
    2020-10-13 barx Summarygtroff hangs in environment::possibly_break_line with -mja => [PATCH] gtroff hangs in environment::possibly_break_line with -mja
    2018-11-26 bgarrigues Item GroupIncorrect behaviour => Crash/Unresponsive
    2018-11-25 bgarrigues CategoryNone => Core
        Item GroupNone => Incorrect behaviour
    2015-01-16 sayama Attached File- => Added sample1.1, #32837
        Attached File- => Added sample2_TP.1, #32838

    Back to the top


    Powered by Savane 3.9