From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f53.google.com (mail-wr1-f53.google.com [209.85.221.53]) by sourceware.org (Postfix) with ESMTPS id CA0553857BBE for ; Tue, 12 Jul 2022 09:30:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CA0553857BBE Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f53.google.com with SMTP id v16so10281179wrd.13 for ; Tue, 12 Jul 2022 02:30:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=E1oYzg+vUbf5IY6EGqKrp2MrgOQn4rDd2fFpwBlaIRA=; b=q8SpAhYAjrf9f8TyYCJ8k0+z+DsfHaGwpwOVQN/KLT9K2APy0AuFUo3hVxdjpwyLsX I04D3z0O9AaP/xig9AYR+fsWpQ8nE8h9RnCZ2whBKuR0T7exxXVkQf4oEJgSTwD6CiXs homgNs+CynPvSJiCrjtRUUDCHxG2VLJxfiRFtDDeDIzY6kH1C6Hj9IzM06wBQs1AvLLP KJBkxlaVAfN6io8j9qujJh465znSAYQXGe1j/sZwvxvXS9uKWt88V3jdCpuIuLm1iE6Z +LGJD74vyFOe/wxoJARQtZWSvJIXTelEY+JnO6buhjU6QwjyJfn0ZyPhnd9+AtbuW0Qd bMLQ== X-Gm-Message-State: AJIora8ljw/RnaiJUjkd8Q986WMEgXMLT1CvkhOL7UkLfmfiNJ44WqbL WmP2PGpvD2tjiniBvrAnBjN/HgQ0xDo= X-Google-Smtp-Source: AGRyM1vRVwC9tjFRa5xAN3ENGaJc086fjkVNTUHKINue25nwhsbJdX4lBkSGg/2erz+CzRM1wgaQ6g== X-Received: by 2002:a05:6000:2cc:b0:21d:76d7:995d with SMTP id o12-20020a05600002cc00b0021d76d7995dmr21852024wry.339.1657618210180; Tue, 12 Jul 2022 02:30:10 -0700 (PDT) Received: from ?IPv6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id f10-20020a7bc8ca000000b003a0231af43csm8712665wml.48.2022.07.12.02.30.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 12 Jul 2022 02:30:09 -0700 (PDT) Subject: Re: [PATCH][gdb/symtab] Fix out-of-bounds in objfile::section_offset To: Tom de Vries , gdb-patches@sourceware.org References: <20220712080032.GA18344@delia.home> From: Pedro Alves Message-ID: Date: Tue, 12 Jul 2022 10:30:03 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220712080032.GA18344@delia.home> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no 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 09:30:18 -0000 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. 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*? 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. 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?