Re: [RFAv2] Fix leak in displaced step.

[Simon Marchi <simon.marchi@ericsson.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5436 bytes --]

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
\x16º&Öéj×!zÊÞ¶êçם¶ëYb²Ö«r\x18\x1dn–­r\x17¬