[PATCH][PR gdb/27133] Avoid use after free with logging and debug redirect

[PATCH][PR gdb/27133] Avoid use after free with logging and debug redirect

[Lancelot SIX via Gdb-patches]

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.

My copyright assignment has not yet been signed by FSF, but the change
should be small enough not to require it. Otherwise, it will need to
wait a few more days I guess.

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.
---
 gdb/cli/cli-interp.c                   | 8 ++++++++
 gdb/testsuite/gdb.base/ui-redirect.exp | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 424264a80e..882384e9a9 100644
--- 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/gdb.base/ui-redirect.exp b/gdb/testsuite/gdb.base/ui-redirect.exp
index 21fbb58aff..e6855d2378 100644
--- a/gdb/testsuite/gdb.base/ui-redirect.exp
+++ b/gdb/testsuite/gdb.base/ui-redirect.exp
@@ -133,3 +133,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\\."
+}
-- 
2.29.2

Re: [PATCH][PR gdb/27133] Avoid use after free with logging and debug redirect

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

Lancelot> My copyright assignment has not yet been signed by FSF, but the change
Lancelot> should be small enough not to require it. Otherwise, it will need to
Lancelot> wait a few more days I guess.

I don't know the state of this -- could you say?

Lancelot> gdb/ChangeLog:

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

Thanks.  Looking at the code I feel like there's probably some cleaner
way to do this, but this patch is ok regardless.

Tom

Re: [PATCH][PR gdb/27133] Avoid use after free with logging and debug redirect

[Lancelot SIX via Gdb-patches]

On 04/01/2021 20:26, Tom Tromey wrote:
>>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
> Lancelot> My copyright assignment has not yet been signed by FSF, but the change
> Lancelot> should be small enough not to require it. Otherwise, it will need to
> Lancelot> wait a few more days I guess.
>
> I don't know the state of this -- could you say?

Hi,

I have signed my end of the document, and am waiting for it to be also 
signed by the FSF. I should be quite close to the end of the procedure I 
guess. I’ll let you know when this is validated. In the meantime, I have 
few patch I still need to update according to reviews.

> Lancelot> gdb/ChangeLog:
>
> Lancelot> 	PR gdb/27133
> Lancelot> 	* cli/cli-interp.c (cli_interp_base::set_logging): Ensure the
> Lancelot> 	unique_ptr is released when the wrapped pointer is kept for later
> Lancelot> 	use.
>
> Thanks.  Looking at the code I feel like there's probably some cleaner
> way to do this, but this patch is ok regardless.

Yes, this feels a bit hacky, but I have gone through few variations and 
up until now none were as straight forward as I’d like them to be (hence 
the bloc of comment.). Here is another approach you might prefer:

---
diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 424264a80e..34afc94d5a 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -417,7 +417,7 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect,
        ui_file *logfile_p = nullptr;
        if (logging_redirect || debug_redirect)
  	{
-	  logfile_p = logfile.get ();
+	  logfile_p = logfile.release ();
  	  saved_output.file_to_delete = logfile_p;
  	}
  
@@ -426,7 +426,13 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect,
        ui_file *tee = nullptr;
        if (!logging_redirect || !debug_redirect)
  	{
-	  tee = new tee_file (gdb_stdout, std::move (logfile));
+          /* If logfile_p not nullptr, this is because it took ownership
+             of the logfile.  In this case transfer the ownership from
+             logfile_p to tee, otherwise just transfer ownership directly
+             from logfile to tee. */
+	  tee = new tee_file (gdb_stdout, (logfile_p == nullptr ?
+                                           std::move (logfile) :
+                                           ui_file_up (logfile_p)));
  	  saved_output.file_to_delete = tee;
  	}
   

This option might makes it easier to follow ownership.

Lancelot

> Tom

Re: [PATCH][PR gdb/27133] Avoid use after free with logging and debug redirect

[Lancelot SIX via Gdb-patches]

Le Mon, Jan 04, 2021 at 01:26:20PM -0700, Tom Tromey a écrit :
> >>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Lancelot> My copyright assignment has not yet been signed by FSF, but the change
> Lancelot> should be small enough not to require it. Otherwise, it will need to
> Lancelot> wait a few more days I guess.
> 
> I don't know the state of this -- could you say?
> 
> Lancelot> gdb/ChangeLog:
> 
> Lancelot> 	PR gdb/27133
> Lancelot> 	* cli/cli-interp.c (cli_interp_base::set_logging): Ensure the
> Lancelot> 	unique_ptr is released when the wrapped pointer is kept for later
> Lancelot> 	use.
> 
> Thanks.  Looking at the code I feel like there's probably some cleaner
> way to do this, but this patch is ok regardless.
> 
> Tom

Hi,

I have pushed this commit.

I cannot close the associated bug on bugzilla[1] because my account does
not have enough privileges (I can only comment on tickets). Could someone
close it for me?

Thanks.
Lancelot.


[1] https://sourceware.org/bugzilla/show_bug.cgi?id=27133