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

[Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>]

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