From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11644 invoked by alias); 19 May 2014 22:31:01 -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 11620 invoked by uid 89); 19 May 2014 22:31:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-vc0-f173.google.com Received: from mail-vc0-f173.google.com (HELO mail-vc0-f173.google.com) (209.85.220.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 19 May 2014 22:30:59 +0000 Received: by mail-vc0-f173.google.com with SMTP id il7so10364478vcb.18 for ; Mon, 19 May 2014 15:30:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:content-type; bh=f8ZF9JnGgm3cEenWLJ7JJ2Amj3dkt665mHqZGpOjXTA=; b=DMIB8c5Njsl8ZZRshw3plpyo9niVYSeZnX3Ujz5R+JvYV0qDFD+L1VsQKEZd7HGPXH vqg57EXOE/R2asl9RvoqMhO1dYDhvwx/+fGPg+pP+uSDs6VRRcxYxE91O2LnYWZZDKSc /RbBuVCsrqvHqLF45U2IEzgO6JLhpWBo35R4M3j+NkXx0Rr3o8Y38NXXZ58i7nmtGZXE E/ntJtvYzdWeeg8EwnJw7I2qeIsIE00twgPYo07gABBlS6BF8LqNpyCWzjgk9yIquCso dWs5qXX6g1WeaLbuf+HwLo3ZG+Qr2UjOBDM3XGHloqo/4LhZClzrbbohi6Bdnk75XWDn RvwA== X-Gm-Message-State: ALoCoQn88Qfoajwff+NFCb/3zIozbGwwqBb8Heef9xO1mwTKOm9vG1Kwj268/0uuaIJEIiwXhgV+ MIME-Version: 1.0 X-Received: by 10.52.125.198 with SMTP id ms6mr472903vdb.28.1400538657006; Mon, 19 May 2014 15:30:57 -0700 (PDT) Received: by 10.52.28.230 with HTTP; Mon, 19 May 2014 15:30:56 -0700 (PDT) In-Reply-To: References: Date: Mon, 19 May 2014 22:31:00 -0000 Message-ID: Subject: Re: [PATCH] Fix gdb.multi/base.exp, gdb memory corruption From: Doug Evans To: gdb-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-05/txt/msg00386.txt.bz2 On Mon, May 19, 2014 at 2:52 PM, Doug Evans wrote: > On Mon, May 19, 2014 at 1:39 PM, Doug Evans wrote: >> Hi. >> >> My prune_inferiors patch >> >> 2014-05-17 Doug Evans >> >> * inferior.c (prune_inferiors): Fix comment. >> (remove_inferior_command): Call prune_program_spaces. >> >> is causing gdb.multi/base.exp to fail: >> >> UNRESOLVED: gdb.multi/base.exp: remove-inferiors 2-3 >> UNRESOLVED: gdb.multi/base.exp: check remove-inferiors >> >> gdb is crashing because it's accessing/freeing already freed memory. >> I was running the testsuite all weekend, sorry I only saw this now. >> >> E.g. >> ==16368== Invalid read of size 4 >> ==16368== at 0x660A9D: find_pc_section (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/objfiles.c:1349) >> ==16368== by 0x663ECB: lookup_minimal_symbol_by_pc_section (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/minsyms.c:734) >> ==16368== by 0x5D987A: find_pc_sect_symtab (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/symtab.c:2153) >> ==16368== by 0x5D4D77: blockvector_for_pc_sect (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/block.c:168) >> ==16368== by 0x5D4F59: block_for_pc_sect (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/block.c:246) >> ==16368== by 0x5D4F9B: block_for_pc (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/block.c:258) >> ==16368== by 0x734C5D: inline_frame_sniffer (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/inline-frame.c:218) >> ==16368== by 0x732104: frame_unwind_try_unwinder (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame-unwind.c:108) >> ==16368== by 0x73223F: frame_unwind_find_by_frame (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame-unwind.c:159) >> ==16368== by 0x72D5AA: compute_frame_id (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame.c:453) >> ==16368== by 0x7300EC: get_prev_frame_if_no_cycle (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame.c:1758) >> ==16368== by 0x73079A: get_prev_frame_always (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame.c:1931) >> ==16368== Address 0x5b13500 is 16 bytes inside a block of size 24 free'd >> ==16368== at 0x403072E: free (valgrind/coregrind/m_replacemalloc/vg_replace_malloc.c:445) >> ==16368== by 0x762134: xfree (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/common/common-utils.c:108) >> ==16368== by 0x65DACF: objfiles_pspace_data_cleanup (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/objfiles.c:91) >> ==16368== by 0x75E546: program_spaceregistry_callback_adaptor (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/progspace.c:45) >> ==16368== by 0x7644F6: registry_clear_data (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/registry.c:82) >> ==16368== by 0x7645AB: registry_container_free_data (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/registry.c:95) >> ==16368== by 0x75E5B4: program_space_free_data (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/progspace.c:45) >> ==16368== by 0x75E9BA: release_program_space (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/progspace.c:167) >> ==16368== by 0x75EB9B: prune_program_spaces (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/progspace.c:269) >> ==16368== by 0x75303D: remove_inferior_command (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/inferior.c:792) >> ==16368== by 0x50B5FD: do_cfunc (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/cli/cli-decode.c:107) >> ==16368== by 0x50E6F2: cmd_func (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/cli/cli-decode.c:1886) >> >> The problem originates from the get_current_arch call in >> py-progspace.c:py_free_pspace. >> I think the comment in this patch explains things pretty well. >> Basically, we're in the pspace destructor, and thus there's not much we can >> rely on. The Python machinery needs an arch, so we give it one, >> albeit a fiction. I think(!) it doesn't matter. >> I'm going with this fix because it's not clear to me what The Right fix >> is, short of removing global state in gdb which is non-trivial. >> Since "there is always an inferior" calling target_gdbarch seems >> pretty safe here. >> >> 2014-05-19 Doug Evans >> >> * python/py-progspace.c (py_free_pspace): Use target_gdbarch instead >> of get_current_arch. >> >> diff --git a/gdb/python/py-progspace.c b/gdb/python/py-progspace.c >> index cda5a86..c787c69 100644 >> --- a/gdb/python/py-progspace.c >> +++ b/gdb/python/py-progspace.c >> @@ -241,7 +241,16 @@ py_free_pspace (struct program_space *pspace, void *datum) >> { >> struct cleanup *cleanup; >> pspace_object *object = datum; >> - struct gdbarch *arch = get_current_arch (); >> + /* This is a fiction, but we're in a nasty spot: The pspace is in the >> + process of being deleted, we can't rely on anything in it. Plus >> + this is one time when the current program space and current inferior >> + are not in sync. The architecture comes from the inferior, which cannot >> + be the current one because we wouldn't be deleting its pspace. >> + We don't need to do much here so this fiction suffices. >> + Note: We cannot call get_current_arch because it may try to access >> + the target, which may involve accessing data in the pspace currently >> + being deleted. */ >> + struct gdbarch *arch = target_gdbarch (); >> >> cleanup = ensure_python_env (arch, current_language); >> object->pspace = NULL; > > Hmmm... > > I looked at py_free_inferior to see what it does: > > ---snip--- > static void > py_free_inferior (struct inferior *inf, void *datum) > { > > struct cleanup *cleanup; > inferior_object *inf_obj = datum; > struct threadlist_entry *th_entry, *th_tmp; > > if (!gdb_python_initialized) > return; > > cleanup = ensure_python_env (python_gdbarch, python_language); > ---snip--- > > Just passing python_gdbarch back into ensure_python_env doesn't feel right. > One might try to get the arch from INF but it is being destructed and > we it would be preferable to not have to rely on it being available. > One solution would be to have a special "dummy" or some such arch for > use during these kinds of situations. > > Plus I see python_on_normal_stop calling get_current_arch whereas > python_on_resume calling target_gdbarch. > There are no comments in the code explaining why things are the way > they are, thus it's hard to reason whether these choices have been > deliberately made or are essentially random. It could be the case > that either choice is fine because it will never be used, but IWBN to > have that documented (or better yet just be consistent and document it > some place). > > It feels like there's a good cleanup project here. > Am I missing something? Curiouser and curiouser ... progspace.c:release_program_space has this: if (!gdbarch_has_shared_address_space (target_gdbarch ())) free_address_space (pspace->aspace); That also can't be right. [Right?] We haven't (and can't) set current_inferior to one that used this program space: it's gone. So target_gdbarch() is essentially returning a random value upon which we will decide whether to call free_address_space. Since it's the arch that decides "has_shared_address_space", and since we're allocating an object stored in a pspace based on this decision, ISTM that a program space also has an arch.