bugGNU roff - Bugs: bug #63873, \~ can cause overwritten text

 
 

bug #63873: \~ can cause overwritten text

Submitter:  Alejandro Colomar <alx>
Submitted:  Mon 06 Mar 2023 02:26:20 PM UTC
   
 
Category:  Core Severity:  3 - Normal
Item Group:  Incorrect behaviour Status:  Duplicate
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 30 Mar 2023 12:08:42 PM UTC, comment #6: 

Closing as duplicate of bug #52517, as Bjarni noted.

If the issues turn out to be distinguishable (which may not be decidable until a fix for one of them is validated), this one can be re-opened.

G. Branden Robinson <gbranden>
Group administrator
Sat 11 Mar 2023 10:38:04 PM UTC, comment #5: 

comment #4:

> a copy/paste is likely to mangle tab characters


Turns out copy/paste is the least of the issues, as savannah mangles tabs--even inside a "verbatim" block--before anyone's mouse can even get to them.  Viewing source on this page shows the tab characters in that patch have been changed to strings of &nbsp; characters.

Dave <barx>
Group Member
Sat 11 Mar 2023 10:08:01 PM UTC, comment #4: 

The first patch in comment #3 displays in savannah incorrectly because savannah interprets two asterisks on the same line of code as starting and ending markers for boldface.  Putting the patch in a +verbatim+ block (see savannah's markup instructions) avoids this problem, and also visually distinguishes the patch from the rest of the comment:

diff --git a/src/roff/troff/env.cpp b/src/roff/troff/env.cpp
index 9f00284c6..0ec4e40e3 100644
--- a/src/roff/troff/env.cpp
+++ b/src/roff/troff/env.cpp
@@ -2081,11 +2081,12 @@ static node *node_list_reverse(node *n)
 static void distribute_space(node *n, int nspaces, hunits desired_space,
                              bool force_reverse_node_list = false)
 {
-  if (desired_space.is_zero() || nspaces == 0)
-    return;
   // Positive desired space is the typical case.  Negative desired space
-  // is possible if we have overrun an unbreakable line.  But we should
-  // not get here if there are no adjustable space nodes to adjust.
+  // is possible if we have overrun an unbreakable line.
+  if (desired_space.to_units() <= 0 || nspaces == 0)
+    return;
+  // But we should not get here if there are no adjustable space nodes
+  // to adjust.
   assert(nspaces > 0);
   // Space cannot always be distributed evenly among all of the space
   // nodes in the node list: there are limits to device resolution.  We
@@ -2098,8 +2099,7 @@ static void distribute_space(node *n, int nspaces, hunits desired_space,
   static bool do_reverse_node_list = false;
   if (force_reverse_node_list || do_reverse_node_list)
     n = node_list_reverse(n);
-  if (!force_reverse_node_list && spread_limit >= 0
-      && desired_space.to_units() > 0) {
+  if (!force_reverse_node_list && spread_limit >= 0) {
     hunits em = curenv->get_size();
     double Ems = (double)desired_space.to_units() / nspaces
                  / (em.is_zero() ? hresolution : em.to_units());

And it's useful to be able to see a patch inline like that...but there's still the problem that a copy/paste is likely to mangle tab characters (though in this case, even if it does, the tabs don't appear on any of the changed lines, and the patch still applies successfully).  So attaching a patch is the safest route.

For the historical record, a variant of this patch, with the same code change but that additionally removes a comment (for reasons I'm not sure of, as the comment still seems relevant to me) was posted to the groff list (http://lists.gnu.org/r/groff/2023-03/msg00036.html) about 45 minutes after comment #3.

Dave <barx>
Group Member
Tue 07 Mar 2023 12:09:41 PM UTC, comment #3: 


comment #2:

>   See bug #52517 (26th November 2017).


Thanks!  Seems a duplicate.  I have a couple of patches that would fix the bug (I think the first one is better, but I'll propose both, since I may miss some groff obscure stuff):

```diff
diff --git a/src/roff/troff/env.cpp b/src/roff/troff/env.cpp
index 9f00284c6..0ec4e40e3 100644
--- a/src/roff/troff/env.cpp
+++ b/src/roff/troff/env.cpp
@@ -2081,11 +2081,12 @@ static node *node_list_reverse(node *n)
 static void distribute_space(node *n, int nspaces, hunits desired_space,
                             bool force_reverse_node_list = false)
 {
-  if (desired_space.is_zero() || nspaces == 0)
-    return;
   // Positive desired space is the typical case.  Negative desired space
-  // is possible if we have overrun an unbreakable line.  But we should
-  // not get here if there are no adjustable space nodes to adjust.
+  // is possible if we have overrun an unbreakable line.
+  if (desired_space.to_units() <= 0 || nspaces == 0)
+    return;
+  // But we should not get here if there are no adjustable space nodes
+  // to adjust.
   assert(nspaces > 0);
   // Space cannot always be distributed evenly among all of the space
   // nodes in the node list: there are limits to device resolution.  We
@@ -2098,8 +2099,7 @@ static void distribute_space(node *n, int nspaces, hunits desired_space,
   static bool do_reverse_node_list = false;
   if (force_reverse_node_list || do_reverse_node_list)
     n = node_list_reverse(n);
-  if (!force_reverse_node_list && spread_limit >= 0
-      && desired_space.to_units() > 0) {
+  if (!force_reverse_node_list && spread_limit >= 0) {
     hunits em = curenv->get_size();
     double Ems = (double)desired_space.to_units() / nspaces
                 / (em.is_zero() ? hresolution : em.to_units());
```


Alternative:

```diff
diff --git a/src/roff/troff/env.cpp b/src/roff/troff/env.cpp
index 9f00284c6..e7724fe30 100644
--- a/src/roff/troff/env.cpp
+++ b/src/roff/troff/env.cpp
@@ -2106,8 +2106,10 @@ static void distribute_space(node *n, int nspaces, hunits desired_space,
     if (Ems > spread_limit)
       output_warning(WARN_BREAK, "spreading %1m per space", Ems);
   }
-  for (node *tem = n; tem; tem = tem->next)
-    tem->spread_space(&nspaces, &desired_space);
+  if (desired_space.to_units() > 0) {
+    for (node *tem = n; tem; tem = tem->next)
+      tem->spread_space(&nspaces, &desired_space);
+  }
   if (force_reverse_node_list || do_reverse_node_list)
     (void)node_list_reverse(n);
   if (!force_reverse_node_list)
```

Alejandro Colomar <alx>
Tue 07 Mar 2023 01:29:47 AM UTC, comment #2: 

  See bug #52517 (26th November 2017).

Bjarni Ingi Gislason <bjarniig>
Mon 06 Mar 2023 02:41:09 PM UTC, comment #1: 

Smaller reproducer:

$ cat space.man
.TH space stretchable to-day experiments
.SH Bad
qwrt\~USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX
.SH Good
$ groff -man -Tutf8 -rLL=60n space.man
troff:space.man:3: warning [p 1, 0.5i]: cannot break line
troff:space.man:5: warning [p 1, 1.0i]: cannot break line
space(stretchable)                        space(stretchable)

Bad
       USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX

Good
       qwrt USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX

experiments                to‐day         space(stretchable)

Alejandro Colomar <alx>
Mon 06 Mar 2023 02:26:20 PM UTC, original submission:  

Reproducible in both 1.22.4 and 1.23.0-HEAD.

Reproducer:

$ cat interval.man
.TH interval notation to-day experiments
.SH Description
What happens if we use interval notation,
with some formatting?
.RB [ 0 ,\~ USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX ]
And make the terminal very short?
.IP
To make things worse, let's add indentation:
.RB [ 1 ,\~ USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX ].
.RS 8
.PP
Let's see with a moved left margin:
.RB [ 2 ,\~ USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX ].
.IP
Why not both things?
.RB [ 3 ,\~ USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX ].
.IP
It seems to be independent of the margin of indentation,
but maybe with a different space things change:
.RB [ 4 ",\ " USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX ].
.IP
Indeed, this last paragraph was better.
It seems top be a bug with \e\[ti].
.RE


See how it renders (admittedly, there are warnings):


$ groff --version | head -n1
GNU groff version 1.23.0.rc3.43-0221b
$ groff -man -Tutf8 -rLL=60n interval.man
troff:interval.man:5: warning [p 1, 0.7i]: cannot adjust line
troff:interval.man:5: warning [p 1, 0.8i]: cannot break line
troff:interval.man:9: warning [p 1, 1.5i]: cannot break line
troff:interval.man:13: warning [p 1, 2.0i]: cannot break line
troff:interval.man:16: warning [p 1, 2.5i]: cannot break line
troff:interval.man:20: warning [p 1, 3.3i]: cannot break line
interval(notation)                        interval(notation)

Description
       What  happens  if we use interval notation, with some
       formatting?
      USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX]
       And make the terminal very short?

              To  make  things worse, let’s add indentation:
     USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX].

               Let’s   see   with   a   moved  left  margin:
     USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX].

                      Why       not       both       things?
     USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX].

                      It seems to be independent of the mar‐
                      gin  of  indentation, but maybe with a
                      different   space    things    change:
                      [4, USHRTrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrtrt_MAX].

                      Indeed,  this  last paragraph was bet‐
                      ter.  It seems top be a bug with \~.

experiments                to‐day         interval(notation)


On 3/6/23 14:47, G. Branden Robinson wrote:

> Just from eyeballing the behavior, it looks like there is no lower bound
> on the size of a "stretchable" ("paddable") space, and there should be.
> Or equivalently, the "stretch" should not be permitted to take negative
> values.

Alejandro Colomar <alx>

 

(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 barx (Posted a comment)
  • -email is unavailable- added by gbranden (Updated the item)
  • -email is unavailable- added by bjarniig (Posted a comment)
  • -email is unavailable- added by alx (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-03-30 gbranden StatusNone Duplicate
        Assigned toNone gbranden
        Open/ClosedOpen Closed
    2023-03-09 gbranden CategoryNone Core
        Item GroupRendering/Cosmetics Incorrect behaviour
        Summary\~ can overwrite previous text. \~ can cause overwritten text

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code