gbranden pushed a commit to branch master
in repository groff.
commit 22b85a62d8ad339e0379c81f173b984a8b65fc3b
Author: G. Branden Robinson <[email protected]>
AuthorDate: Thu Apr 3 05:07:04 2025 -0500
[troff]: Fix Savannah #66981 (use-after-free).
* src/roff/troff/input.cpp (file_iterator::set_location)
(next_file, line_file, do_source, open_file, copy_file)
(transparent_file, do_macro_source): Stop freeing memory dynamically
allocated to store file names encountered in (and copied from) the
input document. *roff is a powerful enough language that the
lifetimes of these file name strings are highly variable, and moreover
their pointers tend to get copied into internal data structures.
Overly aggressive freeing can cause garbage to appear in lieu of file
names in backtrace reports, dumped macros, and so forth. Add comments
musing about a future approach to management of this storage.
Fixes <https://savannah.gnu.org/bugs/?66981>.
---
ChangeLog | 16 ++++++++++++++++
src/roff/troff/div.cpp | 2 ++
src/roff/troff/input.cpp | 24 ++++++++++--------------
3 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 2c3d9383d..4488ccb20 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2025-04-03 G. Branden Robinson <[email protected]>
+
+ * src/roff/troff/input.cpp (file_iterator::set_location)
+ (next_file, line_file, do_source, open_file, copy_file)
+ (transparent_file, do_macro_source): Stop freeing memory
+ dynamically allocated to store file names encountered in (and
+ copied from) the input document. *roff is a powerful enough
+ language that the lifetimes of these file name strings are
+ highly variable, and moreover their pointers tend to get copied
+ into internal data structures. Overly aggressive freeing can
+ cause garbage to appear in lieu of file names in backtrace
+ reports, dumped macros, and so forth. Add comments musing about
+ a future approach to management of this storage.
+
+ Fixes <https://savannah.gnu.org/bugs/?66981>.
+
2025-04-03 G. Branden Robinson <[email protected]>
[groff]: Regression-test Savannah #66981.
diff --git a/src/roff/troff/div.cpp b/src/roff/troff/div.cpp
index 2a978a1ad..bd6461496 100644
--- a/src/roff/troff/div.cpp
+++ b/src/roff/troff/div.cpp
@@ -627,6 +627,8 @@ void end_diversions()
}
}
+// TODO: This might be better named `write_trailer_and_exit()`. Most
+// formatter state is "cleaned up" in input.cpp:exit_troff().
void cleanup_and_exit(int exit_code)
{
if (the_output != 0 /* nullptr */) {
diff --git a/src/roff/troff/input.cpp b/src/roff/troff/input.cpp
index 2455946ee..5b27ce7c4 100644
--- a/src/roff/troff/input.cpp
+++ b/src/roff/troff/input.cpp
@@ -532,13 +532,8 @@ void file_iterator::backtrace()
bool file_iterator::set_location(const char *f, int ln)
{
- if (f != 0 /* nullptr */) {
- if (filename != 0 /* nullptr */) {
- free(filename);
- filename = 0 /* nullptr */;
- }
- filename = strdup(const_cast<char *>(f));
- }
+ if (f != 0 /* nullptr */)
+ filename = const_cast<char *>(f);
lineno = ln;
return true;
}
@@ -935,7 +930,7 @@ void next_file()
else
input_stack::next_file(fp, filename);
}
- delete[] filename;
+ // TODO: Add `filename` to file name set.
tok.next();
}
@@ -2935,6 +2930,7 @@ void exit_troff()
tok.next();
process_input_stack();
}
+ // TODO: delete pointers in file name set.
cleanup_and_exit(EXIT_SUCCESS);
}
@@ -6478,7 +6474,7 @@ void line_file()
if (has_arg(true /* peek */)) {
const char *reported_file_name = read_rest_of_line_as_argument();
(void) input_stack::set_location(reported_file_name, (n - 1));
- delete[] reported_file_name;
+ // TODO: Add `reported_file_name` to file name set.
tok.next();
return;
}
@@ -6860,7 +6856,7 @@ void do_source(bool quietly)
// expected problem.
if (!(quietly && (ENOENT == errno)))
error("cannot open '%1': %2", filename, strerror(errno));
- delete[] filename;
+ // TODO: Add `filename` to file name set.
tok.next();
}
@@ -7678,7 +7674,7 @@ static void open_file(bool appending)
stream_dictionary.define(stream, (object *)grost);
}
}
- delete[] filename;
+ // TODO: Add `filename` to file name set.
tok.next();
}
}
@@ -8764,7 +8760,7 @@ void copy_file()
curenv->do_break();
if (filename != 0 /* nullptr */)
curdiv->copy_file(filename);
- delete[] filename;
+ // TODO: Add `filename` to file name set.
tok.next();
}
@@ -8828,7 +8824,7 @@ void transparent_file()
fclose(fp);
}
}
- delete[] filename;
+ // TODO: Add `filename` to file name set.
tok.next();
}
@@ -8980,7 +8976,7 @@ void do_macro_source(bool quietly)
if (!quietly && (ENOENT == errno))
warning(WARN_FILE, "cannot open macro file '%1': %2",
macro_filename, strerror(errno));
- delete[] macro_filename;
+ // TODO: Add `macro_filename` to file name set.
tok.next();
}
_______________________________________________
groff-commit mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/groff-commit
