Avoid use after free with logging and debug redirect.

This patch addresses PR gdb/27133. Before it, the following succession
of commands would cause gdb to crash:

	set logging redirect on
	set logging debugredirect on
	set logging on

The problem eventually comes down to a use after free. The function
cli_interp_base::set_logging is called with a unique_ptr argument that
holds a pointer to the redirection file. In the problematic use case,
no-one ever took ownership of that pointer (as far as unique_ptr is
concerned), so the call to its dtor at the end of the function causes
the file object to be deleted. Any later use of the pointer to the
redirection file is therefore an error.

This patch ensures that the unique_ptr is released  when required (so it
does not assume ownership anymore). The internal logic of
cli_interp_base::set_logging takes care of freeing the ui_file when it
is not necessary anymore using the saved_output.file_to_delete field.

gdb/ChangeLog:

	PR gdb/27133
	* cli/cli-interp.c (cli_interp_base::set_logging): Ensure the
	unique_ptr is released when the wrapped pointer is kept for later
	use.

gdb/testsuite/ChangeLog:

	PR gdb/27133
	* gdb.base/ui-redirect.exp: Add test case that ensures that
	redirecting both logging and debug does not cause gdb to crash.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 087e2041ba225e63fe764977979d80a9b2a244c9..6cf6d40a247b531b0e750c37e7b3667a611af277 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2021-01-27  Lancelot SIX  <lsix@lancelotsix.com>
+
+       PR gdb/27133
+       * cli/cli-interp.c (cli_interp_base::set_logging): Ensure the
+       unique_ptr is released when the wrapped pointer is kept for later
+       use.
+
 2021-01-27  Matthew Malcomson  <matthew.malcomson@arm.com>
 
        * aarch64-tdep.c (aarch64_displaced_step_others): Account for

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 424264a80ebc7584b7f4759264330a326529d775..882384e9a9155f2c09cc57d9c29bac792654202d 100644 (file)
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -430,6 +430,14 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect,
          saved_output.file_to_delete = tee;
        }
 
+      /* Make sure that the call to logfile's dtor does not delete the
+         underlying pointer if we still keep a reference to it.  If
+         logfile_p is not referenced as the file_to_delete, then either
+         the logfile is not used (no redirection) and it should be
+         deleted, or a tee took ownership of the pointer. */
+      if (logfile_p != nullptr && saved_output.file_to_delete == logfile_p)
+       logfile.release ();
+
       gdb_stdout = logging_redirect ? logfile_p : tee;
       gdb_stdlog = debug_redirect ? logfile_p : tee;
       gdb_stderr = logging_redirect ? logfile_p : tee;

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index f20f041878ee0d1cf8860897abff7d772309c85e..814c6ded1e02307ee230a390cd847f713558ddb5 100644 (file)
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2021-01-27  Lancelot SIX  <lsix@lancelotsix.com>
+
+       PR gdb/27133
+       * gdb.base/ui-redirect.exp: Add test case that ensures that
+       redirecting both logging and debug does not cause gdb to crash.
+
+
 2021-01-27  Matthew Malcomson  <matthew.malcomson@arm.com>
 
        * gdb.arch/insn-reloc.c: Add tests for BR and BLR.

diff --git a/gdb/testsuite/gdb.base/ui-redirect.exp b/gdb/testsuite/gdb.base/ui-redirect.exp
index af428000607051c3b072f601cb4dcc9c419e0f57..cb0749b80c25ec71dcc6a1bd8946e90f37a0765e 100644 (file)
--- a/gdb/testsuite/gdb.base/ui-redirect.exp
+++ b/gdb/testsuite/gdb.base/ui-redirect.exp
@@ -140,3 +140,11 @@ with_test_prefix "redirect debugging" {
     gdb_test "set logging off" "Done logging to /dev/null\\."
     gdb_test "help" "List of classes of commands:.*"
 }
+
+with_test_prefix "redirect logging and debuging" {
+    gdb_test_no_output "set logging redirect on"
+    gdb_test_no_output "set logging debugredirect on"
+    gdb_test "set logging on" \
+    "Redirecting output to /dev/null.*Redirecting debug output to /dev/null\\."
+    gdb_test "set logging off" "Done logging to /dev/null\\."
+}