This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFAv2] Fix leak in displaced step.
- From: David Blaikie <dblaikie at gmail dot com>
- To: Philippe Waroquiers <philippe dot waroquiers at skynet dot be>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 12 Nov 2018 07:34:54 -0800
- Subject: Re: [RFAv2] Fix leak in displaced step.
- References: <20181111121137.6832-1-philippe.waroquiers@skynet.be>
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
>
>