Message ID | 20181111121137.6832-1-philippe.waroquiers@skynet.be |
---|---|
State | New, archived |
Headers |
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: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> 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 <philippe.waroquiers@skynet.be> To: gdb-patches@sourceware.org Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be> 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 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Philippe Waroquiers
Nov. 11, 2018, 12:11 p.m. UTC
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(-)
Comments
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
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);