[bug #62040] [troff] audit mtsm/statem memory usage

[G. Branden Robinson]

Update of bug #62040 (project groff):

                Severity:               2 - Minor => 4 - Important          
              Item Group:                    Lint => Crash/Unresponsive     

    _______________________________________________________

Follow-up Comment #11:

Back in 2018, Jeff Conrad reported a problem with _groff mm_ (the second item
in bug #54909) that seems related to this.

He observed bad vertical spacing in a well-formed _mm_ document when
formatting for the HTML device.

I've trimmed out the other issue, which was a macro package bug for which I
have a fix pending.

[comment #0 original submission:]
> 
> Computer: Core-i7 6700, 32 GB RAM
> OS: Windows 10 Pro, version 1803, 64 bit
> Environment: MKS Toolkit, version 10.0
> groff version: 1.22.3, Windows binary distribution
>             
(https://sourceforge.net/projects/ezwinports/files/groff-1.22.3-3-w32-bin.zip)
> 
> 
> There seem to be a couple of bugs in the mm macro package (file m.tmac):
[...]
> 0  If a list or display follows a heading, there is considerable extra
vertical space between the heading and the list or display.
> 
> I just discovered these issues because for the last 20 years, I was using a
modified version of AT&T mm (which worked fine after I made sure that requests
were followed by whitespace; I may have had to make a few other changes, but
not many).
> 
[...]
> = Extra Vertical Space =
> For the extra vertical space, the problem seems to result from the line
> 
> 
>     \h'\\n[misc*.k]u'\c
> 
> 
> near the end of the macro misc@tag, which is called at the end of macro H. 
This seems to generate something that is forced out when the br request is
given at the beginning of macro SP.
> 
> Offhand, I don’t have a fix.  To be honest, I don’t really understand
what is happening with the generation of HTML tags.  But—at least with
mm—the results I get from Thtml aren’t very good, so I’ve just commented
out the call to misc@tag.  Although it solves my problem, it clearly isn’t a
general solution.

The news gets worse.  In both groff 1.22.4 and Git HEAD, the problem no longer
seems to be (just?) excess vertical space but an outright crash in the
formatter.

Here is a minimal reproducer.  A display has to follow a heading, and the `DE`
call is essential; leave it out, and _mm_'s unclosed diversion handling avoids
the problem.  Also, the display MUST have some content.


$ cat EXPERIMENTS/54909b.mm
.H 1
.DS
display
.DE


The crash does not occur until the formatter is exiting.  A double-free occurs
in the mtsm ("mini-troff state machine") destructor, a feature that is only
used with HTML output.


Core was generated by `troff -b -ww -mm -dwww-image-template=grohtml-9560-
-Thtml'.
Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
##(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f36aceaa537 in __GI_abort () at abort.c:79
#2  0x00007f36acf03768 in __libc_message (action=action@entry=do_abort,
fmt=fmt@entry=0x7f36ad0213a5 "%s\n") at ../sysdeps/posix/libc_fatal.c:155
#3  0x00007f36acf0aa5a in malloc_printerr (str=str@entry=0x7f36ad0236f0
"free(): double free detected in tcache 2") at malloc.c:5347
#4  0x00007f36acf0c055 in _int_free (av=0x7f36ad057b80 <main_arena>,
p=0x5568fb85d5b0, have_lock=0) at malloc.c:4201
#5  0x00005568fab7d5ea in sfree (ptr=<optimized out>) at
../src/libs/libgroff/string.cpp:46
#6  0x00005568fab7d815 in string::~string (this=<optimized out>,
__in_chrg=<optimized out>) at ../src/libs/libgroff/string.cpp:126
#7  0x00005568fab633bb in string_value::~string_value (this=<optimized out>,
__in_chrg=<optimized out>) at ../src/roff/troff/mtsm.cpp:141
#8  0x00005568fab63849 in statem::~statem (this=0x5568fb920a00,
__in_chrg=<optimized out>) at ../src/roff/troff/mtsm.cpp:199
#9  0x00005568fab64184 in stack::~stack (this=0x5568fb85d1d0,
__in_chrg=<optimized out>) at ../src/roff/troff/mtsm.cpp:357
#10 0x00005568fab6424e in mtsm::~mtsm (this=<optimized out>,
__in_chrg=<optimized out>) at ../src/roff/troff/mtsm.cpp:372
#11 0x00005568fab69a31 in output_file::~output_file
(this=this@entry=0x5568fb7ebc20, __in_chrg=<optimized out>) at
../src/roff/troff/node.cpp:1631
#12 0x00005568fab69cf4 in real_output_file::~real_output_file
(this=this@entry=0x5568fb7ebc20, __in_chrg=<optimized out>)
    at ../src/roff/troff/node.cpp:1667
#13 0x00005568fab69d97 in troff_output_file::~troff_output_file
(this=0x5568fb7ebc20, __in_chrg=<optimized out>)
    at ../src/roff/troff/node.cpp:1586
#14 0x00005568fab69dc0 in troff_output_file::~troff_output_file
(this=0x5568fb7ebc20, __in_chrg=<optimized out>)
    at ../src/roff/troff/node.cpp:1589
#15 0x00005568fab3ef97 in cleanup_and_exit (exit_code=exit_code@entry=0) at
../src/roff/troff/div.cpp:567
#16 0x00005568fab3f133 in top_level_diversion::begin_page
(this=this@entry=0x5568fb722b50, n=...) at ../src/roff/troff/div.cpp:582
#17 0x00005568fab3f6a4 in top_level_diversion::space (this=0x5568fb722b50,
n=..., forced=<optimized out>) at ../src/roff/troff/hvunits.h:110
#18 0x00005568fab6034b in exit_troff () at ../src/roff/troff/input.cpp:2616
#19 0x00005568fab60fcd in main (argc=6, argv=0x7fff77b0dad8) at
../src/roff/troff/input.cpp:8279


I instrumented the `mtsm` class to detect a recursive call to the destructor
but did not find one.  My guess at this point is that it is a possible for a
cycle to be created in the stack of `state` objects that `mtsm` maintains.


#9  0x00005568fab64184 in stack::~stack (this=0x5568fb85d1d0,
__in_chrg=<optimized out>) at ../src/roff/troff/mtsm.cpp:357
357         delete state;
##(gdb) list
352     }
353
354     stack::~stack()
355     {
356       if (state)
357         delete state;
358       if (next)
359         delete next;
360     }
361


Somehow _mm_ is able to accomplish this.  I suspect absent input validation
somewhere.

Despite its formal importance I have some doubt whether I'll be able to fix
this for groff 1.23.  Maybe I can add a linear scan to the stack object to
check for previously used pointers and die there; I'm hopeful that the
performance hit will be negligible (and in any event, only -Thtml users will
pay it).  Then the formatter may bomb at a more revealing point when
processing the input, instead of long after, when cleanup and exit is taking
place.

Another possibility is to comment out the same line that Jeff did and see if
that makes the problem go away; if it does, then the foregoing may be
unnecessary, as the culprit will be narrowed down.


    _______________________________________________________

Reply to this item at:

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

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