From patchwork Sun Nov 11 12:11:37 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Philippe Waroquiers X-Patchwork-Id: 30109 Received: (qmail 84940 invoked by alias); 11 Nov 2018 12:11:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 84930 invoked by uid 89); 11 Nov 2018 12:11:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=explains, allocating, common-utils.c, UD:common-utils.c X-HELO: mailsec117.isp.belgacom.be Received: from mailsec117.isp.belgacom.be (HELO mailsec117.isp.belgacom.be) (195.238.20.113) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 11 Nov 2018 12:11:46 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1541938306; x=1573474306; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=93UBRP+6iZfkoZSXRU+/Q3xqsqjmghRYQYEyKO4+f64=; b=R3bKVV8QOSADyNB+XulWRcHChBogTJdpYYlSpUbhQXE7ZwGtUvMl99LO 53wa8vYwtWpV0SpEOSb44Zv1OKEhSg==; Received: from 110.212-243-81.adsl-dyn.isp.belgacom.be (HELO md.home) ([81.243.212.110]) by relay.skynet.be with ESMTP/TLS/DHE-RSA-AES128-GCM-SHA256; 11 Nov 2018 13:11:43 +0100 From: Philippe Waroquiers To: gdb-patches@sourceware.org Cc: Philippe Waroquiers Subject: [RFAv2] Fix leak in displaced step. Date: Sun, 11 Nov 2018 13:11:37 +0100 Message-Id: <20181111121137.6832-1-philippe.waroquiers@skynet.be> MIME-Version: 1.0 X-IsSubscribed: yes 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);