From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14343 invoked by alias); 12 Nov 2018 15:35:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 14319 invoked by uid 89); 12 Nov 2018 15:35:11 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=sk:capture, ownership, HX-Received:a05, H*c:alternative X-HELO: mail-it1-f177.google.com Received: from mail-it1-f177.google.com (HELO mail-it1-f177.google.com) (209.85.166.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 12 Nov 2018 15:35:08 +0000 Received: by mail-it1-f177.google.com with SMTP id k206-v6so13196933ite.0 for ; Mon, 12 Nov 2018 07:35:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=GRUJoDgkm0c70YzBIgoHk1z2hddXIxynTABFIGDuomY=; b=sNXGBsq9qmbdsXAbBvy/10g4sbzdk2mHoT5SiZPXGENpWaJSSkcQlP++KTWBpsOnEP H9XucbK25VLMdeeiWUH3KuZkVhqI+72KSGJcuGh+J11uIg8ndkN5fbFCZXe31kZvE1uO hdPOoUHw793CXfMnzDm58dOsQVU8RSYIa+zuvJJIcLaOpS3yoZQRWtmAnUKyOqFgOsTs ckRb/tP6D3GArNfSHCSFOlMB8Q2lN8U+jpcB+yaFwgbhYpLj8ATFuoX3ARbiAhaWmxqs hWWNxGhHvZ4pfO1SNruQexFzWlTq+wQtjM77SihObnowNlaBq34VcO5gzJ06v0JBkhsv IrDg== MIME-Version: 1.0 References: <20181111121137.6832-1-philippe.waroquiers@skynet.be> In-Reply-To: <20181111121137.6832-1-philippe.waroquiers@skynet.be> From: David Blaikie Date: Mon, 12 Nov 2018 15:35:00 -0000 Message-ID: Subject: Re: [RFAv2] Fix leak in displaced step. To: Philippe Waroquiers Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg00209.txt.bz2 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 > > * 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 > >