gbranden pushed a commit to branch master
in repository groff.
commit b5513742757588e0cf9a043d16dbf06a424906bc
Author: G. Branden Robinson <[email protected]>
AuthorDate: Wed Jun 19 10:38:43 2024 -0500
[troff]: Fix Savannah #65894.
Refactor troff's "mini-troff state machine" to use STL stack
implementation instead of a bespoke one, fixing a double-free error.
* src/roff/troff/mtsm.h: Include C++ standard library "stack" header.
(struct stack): Drop.
(class mtsm): Declare `stack` data member as a standard stack of
`statem` objects instead of a pointer to local `stack` type.
* src/roff/troff/mtsm.h (statem::merge, statem::update):
* src/roff/troff/mtsm.cpp (statem::merge, statem::update): Take
references instead of pointers to some `statem` arguments (those that
will be accessed via the `stack` data member).
* src/roff/troff/mtsm.cpp (stack::stack, stack::~stack): Drop.
(mtsm::mtsm): Drop initializer of `sp` data member.
(mtsm::~mtsm): Drop (conditional) deletion of `sp`.
(mtsm::push_state, mtsm::pop_state, mtsm::inherit): Use STL `stack`'s
`push()`, `empty()`, and `pop()` instead of primitive operations.
(statem::update): Access data members of `mtsm` class via references
instead of pointers (using `.` instead of `->` operator).
(statem::merge): Drop null pointer test of reference, which can't be
null.
Fixes <https://savannah.gnu.org/bugs/?65894>. This was a latent bug
exposed by commit 0951ff53e4, 10 August (a change to the man(7) macro
package; such things should _never_ cause the formatter to crash).
Also drop old-style Emacs file-local variable setting.
The following test fails at this commit:
src/roff/groff/tests/html-does-not-fumble-tagged-paragraph.sh
I attempted a less intrusive change, but did not succeed. The copying
of pointers to groff strings as data members of objects using C++
default copy constructors that also get automatically cleaned up by
destructors also worried me--and the MTSM logic does this sort of thing
a lot--but delegating stack handling to the STL suffices to resolve this
concrete problem. I continue to nurse reservations about the wisdom of
attempting to infer HTML structure from troff requests not designed for
that purpose, the necessity of state management that this approach
imposes, and the robustness of our implementation, which we documented
as being "beta code" about 25 years ago and have changed little since.
I welcome objections to and arguments against my opinion if they're
accompanied by code changes that improve the situation.
---
ChangeLog | 34 ++++++++++++++++++
src/roff/troff/mtsm.cpp | 94 ++++++++++++++++++-------------------------------
src/roff/troff/mtsm.h | 23 +++++-------
3 files changed, 77 insertions(+), 74 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 44fc57bbd..013bd49b9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,37 @@
+2024-06-20 G. Branden Robinson <[email protected]>
+
+ Fix Savannah #65894. Refactor troff's "mini-troff state
+ machine" to use STL stack implementation instead of a bespoke
+ one, fixing a double-free error.
+
+ * src/roff/troff/mtsm.h: Include C++ standard library "stack"
+ header.
+ (struct stack): Drop.
+ (class mtsm): Declare `stack` data member as a standard stack of
+ `statem` objects instead of a pointer to local `stack` type.
+
+ * src/roff/troff/mtsm.h (statem::merge, statem::update):
+ * src/roff/troff/mtsm.cpp (statem::merge, statem::update): Take
+ references instead of pointers to some `statem` arguments (those
+ that will be accessed via the `stack` data member).
+
+ * src/roff/troff/mtsm.cpp (stack::stack, stack::~stack): Drop.
+ (mtsm::mtsm): Drop initializer of `sp` data member.
+ (mtsm::~mtsm): Drop (conditional) deletion of `sp`.
+ (mtsm::push_state, mtsm::pop_state, mtsm::inherit): Use STL
+ `stack`'s `push()`, `empty()`, and `pop()` instead of primitive
+ operations.
+ (statem::update): Access data members of `mtsm` class via
+ references instead of pointers (using `.` instead of `->`
+ operator).
+ (statem::merge): Drop null pointer test of reference, which
+ can't be null.
+
+ Fixes <https://savannah.gnu.org/bugs/?65894>. This was a latent
+ bug exposed by commit 0951ff53e4, 10 August (a change to the
+ man(7) macro package; such things should _never_ cause the
+ formatter to crash).
+
2024-06-20 G. Branden Robinson <[email protected]>
Add regression test for latent bug: surprising "grout" produced
diff --git a/src/roff/troff/mtsm.cpp b/src/roff/troff/mtsm.cpp
index 37d9953d9..05ad91209 100644
--- a/src/roff/troff/mtsm.cpp
+++ b/src/roff/troff/mtsm.cpp
@@ -297,37 +297,37 @@ void statem::add_tag_ta()
}
}
-void statem::update(statem *older, statem *newer, int_value_state t)
+void statem::update(statem &older, statem *newer, int_value_state t)
{
- if (newer->int_values[t].differs(older->int_values[t])
+ if (newer->int_values[t].differs(older.int_values[t])
&& !newer->int_values[t].is_known)
- newer->int_values[t].set(older->int_values[t].value);
+ newer->int_values[t].set(older.int_values[t].value);
}
-void statem::update(statem *older, statem *newer, units_value_state t)
+void statem::update(statem &older, statem *newer, units_value_state t)
{
- if (newer->units_values[t].differs(older->units_values[t])
+ if (newer->units_values[t].differs(older.units_values[t])
&& !newer->units_values[t].is_known)
- newer->units_values[t].set(older->units_values[t].value);
+ newer->units_values[t].set(older.units_values[t].value);
}
-void statem::update(statem *older, statem *newer, bool_value_state t)
+void statem::update(statem &older, statem *newer, bool_value_state t)
{
- if (newer->bool_values[t].differs(older->bool_values[t])
+ if (newer->bool_values[t].differs(older.bool_values[t])
&& !newer->bool_values[t].is_known)
- newer->bool_values[t].set(older->bool_values[t].value);
+ newer->bool_values[t].set(older.bool_values[t].value);
}
-void statem::update(statem *older, statem *newer, string_value_state t)
+void statem::update(statem &older, statem *newer, string_value_state t)
{
- if (newer->string_values[t].differs(older->string_values[t])
+ if (newer->string_values[t].differs(older.string_values[t])
&& !newer->string_values[t].is_known)
- newer->string_values[t].set(older->string_values[t].value);
+ newer->string_values[t].set(older.string_values[t].value);
}
-void statem::merge(statem *newer, statem *older)
+void statem::merge(statem *newer, statem &older)
{
- if (newer == 0 || older == 0)
+ if (0 /* nullptr */ == newer)
return;
update(older, newer, MTSM_EOL);
update(older, newer, MTSM_BR);
@@ -341,26 +341,7 @@ void statem::merge(statem *newer, statem *older)
update(older, newer, MTSM_CE);
}
-stack::stack()
-: next(0), state(0)
-{
-}
-
-stack::stack(statem *s, stack *n)
-: next(n), state(s)
-{
-}
-
-stack::~stack()
-{
- if (state)
- delete state;
- if (next)
- delete next;
-}
-
mtsm::mtsm()
-: sp(0)
{
driver = new statem();
}
@@ -368,8 +349,6 @@ mtsm::mtsm()
mtsm::~mtsm()
{
delete driver;
- if (sp)
- delete sp;
}
/*
@@ -386,7 +365,7 @@ void mtsm::push_state(statem *n)
fflush(stderr);
}
#endif
- sp = new stack(n, sp);
+ stack.push(n);
}
}
@@ -399,13 +378,9 @@ void mtsm::pop_state()
fflush(stderr);
}
#endif
- if (sp == 0)
+ if (stack.empty())
fatal("empty state machine stack");
- sp->state = 0;
- stack *t = sp;
- sp = sp->next;
- t->next = 0;
- delete t;
+ stack.pop();
}
}
@@ -415,36 +390,37 @@ void mtsm::pop_state()
void mtsm::inherit(statem *s, int reset_bool)
{
- if (sp && sp->state) {
+ if (!stack.empty()) {
+ statem top = stack.top();
if (s->units_values[MTSM_IN].is_known
- && sp->state->units_values[MTSM_IN].is_known)
- s->units_values[MTSM_IN].value += sp->state->units_values[MTSM_IN].value;
- s->update(sp->state, s, MTSM_FI);
- s->update(sp->state, s, MTSM_LL);
- s->update(sp->state, s, MTSM_PO);
- s->update(sp->state, s, MTSM_RJ);
- s->update(sp->state, s, MTSM_TA);
- s->update(sp->state, s, MTSM_TI);
- s->update(sp->state, s, MTSM_CE);
- if (sp->state->bool_values[MTSM_BR].is_known
- && sp->state->bool_values[MTSM_BR].value) {
+ && top.units_values[MTSM_IN].is_known)
+ s->units_values[MTSM_IN].value += top.units_values[MTSM_IN].value;
+ s->update(top, s, MTSM_FI);
+ s->update(top, s, MTSM_LL);
+ s->update(top, s, MTSM_PO);
+ s->update(top, s, MTSM_RJ);
+ s->update(top, s, MTSM_TA);
+ s->update(top, s, MTSM_TI);
+ s->update(top, s, MTSM_CE);
+ if (top.bool_values[MTSM_BR].is_known
+ && top.bool_values[MTSM_BR].value) {
if (reset_bool)
- sp->state->bool_values[MTSM_BR].set(0);
+ top.bool_values[MTSM_BR].set(0);
s->bool_values[MTSM_BR].set(1);
#if defined(DEBUGGING)
if (want_html_debugging)
fprintf(stderr, "inherited br from pushed state %d\n",
- sp->state->issue_no);
+ top.issue_no);
#endif
}
else if (s->bool_values[MTSM_BR].is_known
&& s->bool_values[MTSM_BR].value)
if (! s->int_values[MTSM_CE].is_known)
s->bool_values[MTSM_BR].unset();
- if (sp->state->bool_values[MTSM_EOL].is_known
- && sp->state->bool_values[MTSM_EOL].value) {
+ if (top.bool_values[MTSM_EOL].is_known
+ && top.bool_values[MTSM_EOL].value) {
if (reset_bool)
- sp->state->bool_values[MTSM_EOL].set(0);
+ top.bool_values[MTSM_EOL].set(0);
s->bool_values[MTSM_EOL].set(1);
}
}
diff --git a/src/roff/troff/mtsm.h b/src/roff/troff/mtsm.h
index cfca73dce..7981d25d8 100644
--- a/src/roff/troff/mtsm.h
+++ b/src/roff/troff/mtsm.h
@@ -1,4 +1,3 @@
-// -*- C++ -*-
/* Copyright (C) 2003-2020 Free Software Foundation, Inc.
*
* mtsm.h
@@ -25,6 +24,8 @@ for more details.
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
+#include <stack>
+
struct int_value {
int value;
int is_known;
@@ -99,7 +100,7 @@ struct statem {
~statem();
void flush(FILE *, statem *);
int changed(statem *);
- void merge(statem *, statem *);
+ void merge(statem *, statem &);
void add_tag(int_value_state, int);
void add_tag(bool_value_state);
void add_tag(units_value_state, hunits);
@@ -108,23 +109,15 @@ struct statem {
void add_tag_if_unknown(int_value_state, int);
void add_tag_ta();
void display_state();
- void update(statem *, statem *, int_value_state);
- void update(statem *, statem *, bool_value_state);
- void update(statem *, statem *, units_value_state);
- void update(statem *, statem *, string_value_state);
-};
-
-struct stack {
- stack *next;
- statem *state;
- stack();
- stack(statem *, stack *);
- ~stack();
+ void update(statem &, statem *, int_value_state);
+ void update(statem &, statem *, bool_value_state);
+ void update(statem &, statem *, units_value_state);
+ void update(statem &, statem *, string_value_state);
};
class mtsm {
statem *driver;
- stack *sp;
+ std::stack<statem> stack;
int has_changed(int_value_state, statem *);
int has_changed(bool_value_state, statem *);
int has_changed(units_value_state, statem *);
_______________________________________________
Groff-commit mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/groff-commit
