[RFAv2] Fix leak in displaced step.

  Valgrind reports a definite leak of displaced->step_saved_copy
(full leak stack trace below).

This patch fixes the leak by only allocating a new step_saved_copy
if the process displaced_step_inferior_state does not yet have one,
and by freeing it when the displaced_step_inferior_state of a process
is removed, when the inferior exits.

Regtested on debian/amd64 + step-over-syscall.exp rerun under valgrind.

==4736== VALGRIND_GDB_ERROR_BEGIN
==4736== 128 bytes in 8 blocks are definitely lost in loss record 980 of 3,108
==4736==    at 0x4C2BE2D: malloc (vg_replace_malloc.c:299)
==4736==    by 0x41B627: xmalloc (common-utils.c:44)
==4736==    by 0x50D4E3: displaced_step_prepare_throw (infrun.c:1837)
==4736==    by 0x50D4E3: displaced_step_prepare (infrun.c:1898)
==4736==    by 0x50D4E3: resume_1 (infrun.c:2545)
==4736==    by 0x50D4E3: resume(gdb_signal) (infrun.c:2741)
==4736==    by 0x50DCD5: keep_going_pass_signal(execution_control_state*) (infrun.c:7793)
==4736==    by 0x50E903: process_event_stop_test(execution_control_state*) (infrun.c:6843)
==4736==    by 0x510925: handle_signal_stop(execution_control_state*) (infrun.c:6176)
==4736==    by 0x513F79: handle_inferior_event_1 (infrun.c:5354)
==4736==    by 0x513F79: handle_inferior_event(execution_control_state*) (infrun.c:5389)
==4736==    by 0x51541B: fetch_inferior_event(void*) (infrun.c:3916)
==4736==    by 0x4B3EEC: gdb_wait_for_event(int) (event-loop.c:859)
==4736==    by 0x4B3FF6: gdb_do_one_event() [clone .part.4] (event-loop.c:322)
==4736==    by 0x4B41B4: gdb_do_one_event (common-exceptions.h:219)
==4736==    by 0x4B41B4: start_event_loop() (event-loop.c:371)
==4736==    by 0x551237: captured_command_loop() (main.c:330)
==4736==    by 0x55222C: captured_main (main.c:1177)
==4736==    by 0x55222C: gdb_main(captured_main_args*) (main.c:1193)
==4736==    by 0x29B4F7: main (gdb.c:32)
==4736==
==4736== VALGRIND_GDB_ERROR_END

gdb/ChangeLog

2018-11-11  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* infrun.c (displaced_step_inferior_state): Explain why step_saved_copy
	is sometimes needed after the step-over is finished.
	(remove_displaced_stepping_state): xfree step_saved_copy.
	(displaced_step_clear): Add note that explains why we do not xfree
	step_saved_copy here.
---
 gdb/infrun.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)
  

Any chance this could use unique_xmalloc_ptr to reduce the risk of leaking
a bit? (I guess that'd probably mean handling the ownership of
displaced_step_inferorior_state, since it'd then have a non-trivial dtor,
so it couldn't just be xmalloc/xfree'd and would need similar
unique_xmalloc_ptr handling, etc?)

On Sun, Nov 11, 2018 at 4:11 AM Philippe Waroquiers <
philippe.waroquiers@skynet.be> wrote:

> Valgrind reports a definite leak of displaced->step_saved_copy
> (full leak stack trace below).
>
> This patch fixes the leak by only allocating a new step_saved_copy
> if the process displaced_step_inferior_state does not yet have one,
> and by freeing it when the displaced_step_inferior_state of a process
> is removed, when the inferior exits.
>
> Regtested on debian/amd64 + step-over-syscall.exp rerun under valgrind.
>
> ==4736== VALGRIND_GDB_ERROR_BEGIN
> ==4736== 128 bytes in 8 blocks are definitely lost in loss record 980 of
> 3,108
> ==4736==    at 0x4C2BE2D: malloc (vg_replace_malloc.c:299)
> ==4736==    by 0x41B627: xmalloc (common-utils.c:44)
> ==4736==    by 0x50D4E3: displaced_step_prepare_throw (infrun.c:1837)
> ==4736==    by 0x50D4E3: displaced_step_prepare (infrun.c:1898)
> ==4736==    by 0x50D4E3: resume_1 (infrun.c:2545)
> ==4736==    by 0x50D4E3: resume(gdb_signal) (infrun.c:2741)
> ==4736==    by 0x50DCD5: keep_going_pass_signal(execution_control_state*)
> (infrun.c:7793)
> ==4736==    by 0x50E903: process_event_stop_test(execution_control_state*)
> (infrun.c:6843)
> ==4736==    by 0x510925: handle_signal_stop(execution_control_state*)
> (infrun.c:6176)
> ==4736==    by 0x513F79: handle_inferior_event_1 (infrun.c:5354)
> ==4736==    by 0x513F79: handle_inferior_event(execution_control_state*)
> (infrun.c:5389)
> ==4736==    by 0x51541B: fetch_inferior_event(void*) (infrun.c:3916)
> ==4736==    by 0x4B3EEC: gdb_wait_for_event(int) (event-loop.c:859)
> ==4736==    by 0x4B3FF6: gdb_do_one_event() [clone .part.4]
> (event-loop.c:322)
> ==4736==    by 0x4B41B4: gdb_do_one_event (common-exceptions.h:219)
> ==4736==    by 0x4B41B4: start_event_loop() (event-loop.c:371)
> ==4736==    by 0x551237: captured_command_loop() (main.c:330)
> ==4736==    by 0x55222C: captured_main (main.c:1177)
> ==4736==    by 0x55222C: gdb_main(captured_main_args*) (main.c:1193)
> ==4736==    by 0x29B4F7: main (gdb.c:32)
> ==4736==
> ==4736== VALGRIND_GDB_ERROR_END
>
> gdb/ChangeLog
>
> 2018-11-11  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>
>         * infrun.c (displaced_step_inferior_state): Explain why
> step_saved_copy
>         is sometimes needed after the step-over is finished.
>         (remove_displaced_stepping_state): xfree step_saved_copy.
>         (displaced_step_clear): Add note that explains why we do not xfree
>         step_saved_copy here.
> ---
>  gdb/infrun.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 9473d1f20f..1c40cb2e0f 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1510,7 +1510,13 @@ struct displaced_step_inferior_state
>       made.  */
>    CORE_ADDR step_original, step_copy;
>
> -  /* Saved contents of copy area.  */
> +  /* Saved contents of copy area.  In most cases, we could get rid
> +     of this copy when the displaced single-step is finished, after
> +     having restored the content, when setting step_thread to nullptr.
> +     However, we need to keep this content in case the step-over is
> +     over a fork syscall: in such a case, the step-over was done in
> +     the parent, but we also have to restore the copy area content
> +     in the child, after the parent has finished the step-over.  */
>    gdb_byte *step_saved_copy;
>  };
>
> @@ -1638,6 +1644,11 @@ remove_displaced_stepping_state (inferior *inf)
>        if (it->inf == inf)
>         {
>           *prev_next_p = it->next;
> +         if (it->step_saved_copy != NULL)
> +           {
> +             xfree (it->step_saved_copy);
> +             it->step_saved_copy = NULL;
> +           }
>           xfree (it);
>           return;
>         }
> @@ -1709,6 +1720,10 @@ displaced_step_clear (struct
> displaced_step_inferior_state *displaced)
>
>    delete displaced->step_closure;
>    displaced->step_closure = NULL;
> +
> +  /* Note: we cannot xfree (displaced->step_saved_copy), as this
> +     is needed to restore the content in the child, if
> +     the step-over was over a fork syscall.  */
>  }
>
>  static void
> @@ -1834,7 +1849,11 @@ displaced_step_prepare_throw (thread_info *tp)
>      }
>
>    /* Save the original contents of the copy area.  */
> -  displaced->step_saved_copy = (gdb_byte *) xmalloc (len);
> +  if (displaced->step_saved_copy == NULL)
> +    displaced->step_saved_copy = (gdb_byte *) xmalloc (len);
> +  /* Even if we have not allocated step_saved_copy now, make a
> +     (temporary) cleanup for it, in case the setup below fails to
> +     complete the copy.  */
>    ignore_cleanups = make_cleanup (free_current_contents,
>                                   &displaced->step_saved_copy);
>    status = target_read_memory (copy, displaced->step_saved_copy, len);
> --
> 2.19.1
>
>
  

On 2018-11-11 7:11 a.m., Philippe Waroquiers wrote:
> Valgrind reports a definite leak of displaced->step_saved_copy

> (full leak stack trace below).

> 

> This patch fixes the leak by only allocating a new step_saved_copy

> if the process displaced_step_inferior_state does not yet have one,

> and by freeing it when the displaced_step_inferior_state of a process

> is removed, when the inferior exits.

> 

> Regtested on debian/amd64 + step-over-syscall.exp rerun under valgrind.

> 

> ==4736== VALGRIND_GDB_ERROR_BEGIN

> ==4736== 128 bytes in 8 blocks are definitely lost in loss record 980 of 3,108

> ==4736==    at 0x4C2BE2D: malloc (vg_replace_malloc.c:299)

> ==4736==    by 0x41B627: xmalloc (common-utils.c:44)

> ==4736==    by 0x50D4E3: displaced_step_prepare_throw (infrun.c:1837)

> ==4736==    by 0x50D4E3: displaced_step_prepare (infrun.c:1898)

> ==4736==    by 0x50D4E3: resume_1 (infrun.c:2545)

> ==4736==    by 0x50D4E3: resume(gdb_signal) (infrun.c:2741)

> ==4736==    by 0x50DCD5: keep_going_pass_signal(execution_control_state*) (infrun.c:7793)

> ==4736==    by 0x50E903: process_event_stop_test(execution_control_state*) (infrun.c:6843)

> ==4736==    by 0x510925: handle_signal_stop(execution_control_state*) (infrun.c:6176)

> ==4736==    by 0x513F79: handle_inferior_event_1 (infrun.c:5354)

> ==4736==    by 0x513F79: handle_inferior_event(execution_control_state*) (infrun.c:5389)

> ==4736==    by 0x51541B: fetch_inferior_event(void*) (infrun.c:3916)

> ==4736==    by 0x4B3EEC: gdb_wait_for_event(int) (event-loop.c:859)

> ==4736==    by 0x4B3FF6: gdb_do_one_event() [clone .part.4] (event-loop.c:322)

> ==4736==    by 0x4B41B4: gdb_do_one_event (common-exceptions.h:219)

> ==4736==    by 0x4B41B4: start_event_loop() (event-loop.c:371)

> ==4736==    by 0x551237: captured_command_loop() (main.c:330)

> ==4736==    by 0x55222C: captured_main (main.c:1177)

> ==4736==    by 0x55222C: gdb_main(captured_main_args*) (main.c:1193)

> ==4736==    by 0x29B4F7: main (gdb.c:32)

> ==4736==

> ==4736== VALGRIND_GDB_ERROR_END

> 

> gdb/ChangeLog

> 

> 2018-11-11  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

> 

> 	* infrun.c (displaced_step_inferior_state): Explain why step_saved_copy

> 	is sometimes needed after the step-over is finished.

> 	(remove_displaced_stepping_state): xfree step_saved_copy.

> 	(displaced_step_clear): Add note that explains why we do not xfree

> 	step_saved_copy here.

> ---

>  gdb/infrun.c | 23 +++++++++++++++++++++--

>  1 file changed, 21 insertions(+), 2 deletions(-)

> 

> diff --git a/gdb/infrun.c b/gdb/infrun.c

> index 9473d1f20f..1c40cb2e0f 100644

> --- a/gdb/infrun.c

> +++ b/gdb/infrun.c

> @@ -1510,7 +1510,13 @@ struct displaced_step_inferior_state

>       made.  */

>    CORE_ADDR step_original, step_copy;

>  

> -  /* Saved contents of copy area.  */

> +  /* Saved contents of copy area.  In most cases, we could get rid

> +     of this copy when the displaced single-step is finished, after

> +     having restored the content, when setting step_thread to nullptr.

> +     However, we need to keep this content in case the step-over is

> +     over a fork syscall: in such a case, the step-over was done in

> +     the parent, but we also have to restore the copy area content

> +     in the child, after the parent has finished the step-over.  */

>    gdb_byte *step_saved_copy;

>  };

>  

> @@ -1638,6 +1644,11 @@ remove_displaced_stepping_state (inferior *inf)

>        if (it->inf == inf)

>  	{

>  	  *prev_next_p = it->next;

> +	  if (it->step_saved_copy != NULL)

> +	    {

> +	      xfree (it->step_saved_copy);

> +	      it->step_saved_copy = NULL;

> +	    }

>  	  xfree (it);

>  	  return;

>  	}

> @@ -1709,6 +1720,10 @@ displaced_step_clear (struct displaced_step_inferior_state *displaced)

>  

>    delete displaced->step_closure;

>    displaced->step_closure = NULL;

> +

> +  /* Note: we cannot xfree (displaced->step_saved_copy), as this

> +     is needed to restore the content in the child, if

> +     the step-over was over a fork syscall.  */

>  }

>  

>  static void

> @@ -1834,7 +1849,11 @@ displaced_step_prepare_throw (thread_info *tp)

>      }

>  

>    /* Save the original contents of the copy area.  */

> -  displaced->step_saved_copy = (gdb_byte *) xmalloc (len);

> +  if (displaced->step_saved_copy == NULL)

> +    displaced->step_saved_copy = (gdb_byte *) xmalloc (len);

> +  /* Even if we have not allocated step_saved_copy now, make a

> +     (temporary) cleanup for it, in case the setup below fails to

> +     complete the copy.  */

>    ignore_cleanups = make_cleanup (free_current_contents,

>  				  &displaced->step_saved_copy);

>    status = target_read_memory (copy, displaced->step_saved_copy, len);

> 


From what I understand, the allocation model you propose in this patch is to
allocate a buffer the first time we do a displaced step for an inferior and
free it when the inferior exits.  The allocated size is

  len = gdbarch_max_insn_length (gdbarch);

Given that there can be multiple architectures inside a single inferior, can
the required buffer size change between multiple displaced step?

Also, if freeing the buffer on inferior exit is indeed what we want to do, why do
we need the above cleanup?  Even if the setup fails, shouldn't be fine to keep the buffer
allocated?

Simon
  

On 2018-11-12 10:34 a.m., David Blaikie wrote:
> Any chance this could use unique_xmalloc_ptr to reduce the risk of leaking

> a bit? (I guess that'd probably mean handling the ownership of

> displaced_step_inferorior_state, since it'd then have a non-trivial dtor,

> so it couldn't just be xmalloc/xfree'd and would need similar

> unique_xmalloc_ptr handling, etc?)


I think it would be the right way, yes.  If we need the buffer to be resizable,
we could use a vector (gdb::byte_vector), but in any case
displaced_step_inferior_state would need the same treatment.

Simon
  

On Mon, 2018-11-12 at 16:17 +0000, Simon Marchi wrote:
> From what I understand, the allocation model you propose in this patch is to
> allocate a buffer the first time we do a displaced step for an inferior and
> free it when the inferior exits.
Yes, that is the plan.

>   The allocated size is
> 
>   len = gdbarch_max_insn_length (gdbarch);
> 
> Given that there can be multiple architectures inside a single inferior, can
> the required buffer size change between multiple displaced step?
Good remark : I did not know of this multiple architecture for a
single inferior, and then yes possibly the buffer might have to change size.
So, we should rather do (unconditionally) :
  displaced->step_saved_copy = (gdb_byte *) xrealloc (len);

> 
> Also, if freeing the buffer on inferior exit is indeed what we want to do, why do
> we need the above cleanup?  Even if the setup fails, shouldn't be fine to keep the buffer
> allocated?
The idea was to avoid having a piece of memory containing not properly 
initialized data. But probably this is not a problem, as this data will
only be used if the displaced setup is finished, at the end of the function.
So, effectively no real need for the cleanup anymore.

Philippe