From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id D4A413858C50 for ; Tue, 12 Jul 2022 12:09:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D4A413858C50 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 20B6B1FD6B; Tue, 12 Jul 2022 12:09:45 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 080D413638; Tue, 12 Jul 2022 12:09:45 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id +9PHAIlkzWLJHQAAMHmgww (envelope-from ); Tue, 12 Jul 2022 12:09:45 +0000 Message-ID: <64a158d7-ea7f-303a-939a-bc500eee9cdd@suse.de> Date: Tue, 12 Jul 2022 14:09:44 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH][gdb/symtab] Fix out-of-bounds in objfile::section_offset Content-Language: en-US To: Pedro Alves , gdb-patches@sourceware.org References: <20220712080032.GA18344@delia.home> <98108218-5cc6-fab8-fe17-319d37e8cb39@suse.de> <53735898-5c00-1af6-c09a-7cc4622b64f7@palves.net> From: Tom de Vries In-Reply-To: <53735898-5c00-1af6-c09a-7cc4622b64f7@palves.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jul 2022 12:09:47 -0000 On 7/12/22 12:25, Pedro Alves wrote: > On 2022-07-12 11:16 a.m., Tom de Vries wrote: >> On 7/12/22 11:30, Pedro Alves wrote: >>> On 2022-07-12 9:00 a.m., Tom de Vries via Gdb-patches wrote: >>>> Hi, >>>> >>>> Using this patch in objfile::section_offset that checks that idx is within >>>> bounds: >>>> ... >>>>       int idx = gdb_bfd_section_index (this->obfd, section); >>>> +    gdb_assert (idx < section_offsets.size ()); >>>>       return this->section_offsets[idx]; >>>> ... >>>> we run into the assert in test-cases: >>>> - gdb.base/readline-ask.exp >>>> - gdb.base/symbol-without-target_section.exp >>>> - gdb.dwarf2/dw2-icc-opaque.exp >>>> >>>> These were previously reported as -fsanitize-threads issues (PR25724, >>>> PR25723). >>>> >>>> In the case of the latter test-case the problem happens as follows. >>>> >>>> - We start out with bfd_count_sections () == 6, so >>>>    gdb_bfd_count_sections () == 10.  The difference of 4 is due to the >>>>    4 'special sections' named *ABS*, *UND*, *COM* and *IND*. >>>> - According to gdb_bfd_section_index, the *IND* has section index >>>>    bfd_count_sections () + 3, so 9. >>>> - default_symfile_relocate gets called, which calls >>>>    bfd_simple_get_relocated_section_contents and eventually this results in >>>>    bfd_make_section_old_way being called for a section named COMMON, >>>>    meaning now we have bfd_count_sections () == 7 >>>> - consequently the next time we call objfile::section_offset for *IND*, >>>>    gdb_bfd_section_index assigns it section index 10. >>>> - the assert fails because idx == 10 and section_offsets.size () == 10. >>>> >>>> Fix this in a minimal and contained way, by: >>>> - adding a side-table orig_bfd_count_sections_map, in which we store the >>>>    original bfd_count_sections () value, and >>>> - using this value in gdb_bfd_count_sections and gdb_bfd_section_index, >>>>    ensuring that the creation of the new section doesn't interfere with >>>>    accessing the unchanged objfile::sections and objfile::section_offsets. >>>> >>>> In case we call gdb_bfd_section_index with the new section, we assert. >>>> >>>> However, in case gdb_bfd_section_index is not used, and the bfd section index >>>> of the new section is used to access objfile::sections or >>>> objfile::section_offsets, we return some unrelated element, which might fail >>>> in some difficult to understand manner.  It's hard to check whether this can >>>> happen or not without having distinct types for the different section indices >>>> (bfd vs. gdb_bfd).  Anyway, if this does occur, it's a pre-existing bug.  This >>>> patch focuses on getting things right for the original sections. >>>> >>>> Tested on x86_64-linux, with and without -fsanitize=threads. >>>> >>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29295 >>>> >>>> Any comments? >>> >>> Not sure about this, it seems a bit too hacky to me. >> >> I agree, it's far from ideal, and its only merit seems to me that it improves upon the current situation. >> >>> Doesn't this mean that gdb_bfd_section_index >>> ends up returning the same index for two different sections? > Like, in your example above, it returns 6 >>> for both the new COMMON section added by bfd_simple_get_relocated_section_contents and *ABS*? >>> >> >> That's not the case. >> >> So, we have count == 6, as per: >> ... >>   int count = get_orig_bfd_count_sections (abfd); >> ... >> >> For *ABS*, it returns 8, as per: >> ... >>   else if (section == bfd_abs_section_ptr) >>     return count + 2;               .... >> >> Perhaps you mean *COM*, for which it returns 6: >> ... >>   else if (section == bfd_com_section_ptr) >>     return count;               ... >> >> Anyway, for COMMON, with bfd section index 6, it asserts: >> ... >> +  gdb_assert (section->index < count);               ... >> >>> If the count of bfd sections can grow behind our backs, couldn't we solve the index problem >>> by giving sections *ABS*, *UND*, *COM* and *IND* indexes 0 through 3, and then the >>> non-absolute bfd sections would start at 4 ?  I.e., there would be a bias >>> of 4 between gdb section numbers and bfd section numbers, but maybe that wouldn't >>> be a real problem?  This way, if bfd sections grow, the preexisting >>> absolute section indexes would remain stable. >>> >> >> Yes, I tried that, I didn't get that to work.  I suppose it'll require using gdb_bfd_section_index in a lot more places.  And where to use it and where not is not easy to see if both the bfd section index and the gdb_bfd section index are the same type.  I've also tried making those different types without implicit conversion, but also didn't manage to drive that to completion. >> >>> Also, don't we end up with the objfile->sections array with one section too few?  Like, won't it >>> be missing a slot for the new COMMON bfd section?  Are we growing that array somewhere after >>> default_symfile_relocate is called? >> >> AFAIU, neither the sections and sections_offsets array are grown.  I've also looked into fixing that but am not familiar enough with the code to understand what to put in the sections_offset array. > > Another question is, why do the bfd sections grow in the first place? Maybe that's a bfd bug? Like, why > isn't COMMON already in the bfd sections list when the bfd is first opened? Maybe that could be an angle > to tackle this. Yes, that is a possibility. I see in bfd/MAINTAINERS that the binutils maintainers maintain it, so we could ask, perhaps at bug-binutils. Thanks, - Tom