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 BACB13858C50 for ; Tue, 12 Jul 2022 10:16:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BACB13858C50 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 A008D1FA88; Tue, 12 Jul 2022 10:16:16 +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 8C12613638; Tue, 12 Jul 2022 10:16:16 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id ASc0IfBJzWISZwAAMHmgww (envelope-from ); Tue, 12 Jul 2022 10:16:16 +0000 Message-ID: <98108218-5cc6-fab8-fe17-319d37e8cb39@suse.de> Date: Tue, 12 Jul 2022 12:16:16 +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> From: Tom de Vries In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, 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 10:16:20 -0000 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. Thanks, - Tom