Update of bug #65322 (group groff):
Status: Need Info => Fixed
Open/Closed: Open => Closed
Planned Release: None => 1.24.0
Summary: [troff] chopping box diversions can crash the
formatter => [troff] asciifying a chopped box diversion can crash the
formatter
_______________________________________________________
Follow-up Comment #11:
Unfortunately for a root-cause analysis, commit 33f557eceb was later further
refactored. Not long afterward, `node` became an abstract class.
commit 1a38b319a23da6160bfc3009bc12717a01f2614b
Author: G. Branden Robinson <[email protected]>
Date: Thu Sep 18 00:02:09 2025 -0500
[troff]: Refactor `node` class hierarchy.
Make `node` an abstract class. The `asciify` feature had several bugs,
and I think it's because the class design permitted derived classes to
reuse the `node` base class's `asciify` member function, which
robotically appended the `this` object to the macro pointed to in the
function argument. In most cases, the correct "asciification" of a node
is to do nothing, discarding it. Rather than making the member function
empty in the base class, I prefer to make the base class abstract to
force conscious consideration of how each derived class should be
"asciified".
* src/roff/troff/node.h: Mark `add_char()`, `asciify()`, and
`add_italic_correction()` member functions as `virtual`. Further mark
`asciify` as _pure_ virtual (notation: "= 0").
* src/roff/troff/node.cpp (node::asciify): Delete.
And, more specifically, the deletion that was added in the commit in commit
#10 was reverted less than a month later (yet it didn't, per a test I just did
by checking out and building the below, resurrect the core dump).
commit 553ff83c8996d0370116450ea070233344444116
Author: G. Branden Robinson <[email protected]>
Date: Wed Sep 17 18:56:01 2025 -0500
[troff]: Refactor `asciify` internals. (1/3)
* src/roff/troff/node.cpp (glyph_node::asciify)
(kern_pair_node::asciify, dbreak_node::asciify)
(asciify_reverse_node_list, ligature_node::asciify)
(break_char_node::asciify, italic_corrected_node::asciify)
(left_italic_corrected_node::asciify)
(space_char_hmotion_node::asciify, space_node::asciify)
(word_space_node::asciify, unbreakable_space_node::asciify)
(line_start_node::asciify, vertical_size_node::asciify)
(dummy_node::asciify, transparent_dummy_node::asciify)
(tag_node::asciify, device_extension_node::asciify)
(vmotion_node::asciify, bracket_node::asciify)
(hyphen_inhibitor_node::asciify, composite_node::asciify): Stop
deleting the `this` object. I added many of these deletions recently,
but several (`glyph_node`, `kern_pair_node`, `dbreak_node`,
`ligature_node`, `break_char_node`, `italic_corrected_node`,
`left_italic_corrected_node`, `hmotion_node`,
`space_char_hmotion_node`, `space_node`, `word_space_node`,
`unbreakable_space_node`, `line_start_node`, `vertical_size_node`, and
`composite_node`) already were in groff 1.23.0, and for untold years
before--sometimes only conditionally depending on the node contents.
Why stop deleting them? Because the node list is actually list of
(potential) trees; some nodes can contain further nodes, and so on
recursively. It's difficult, and there is no need to, mark the root
of each tree in the list so that we can return to it later to delete
it; instead what we can do is have the `asciify` request walk the list
again and perform a delete operation, which due to the class hierarchy
design will automatically be a complete, recursive operation. There
is no reason _not_ to do this because the whole point of `asciify` is
to convert nodes back into some form of text; the idiomatic
application (and only one, as seen in "om.tmac" and "e.tmac") is to
convert strings or diversions into PDF bookmarks or HTML URLs that are
embedded in a document as metadata or markup, not as formatted text.
I'll chalk this ticket up as resolved by the general rethink and refactoring
of the `node` class hierarchy and the attention I gave to ensuring that the
`asciify()` operation on every class in that hierarchy is well-defined.
Any remaining problems with the `chop` request can be addressed in bug #67453.
_______________________________________________________
Reply to this item at:
<https://savannah.gnu.org/bugs/?65322>
_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/
signature.asc
Description: PGP signature
