From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) by sourceware.org (Postfix) with ESMTPS id C8C583858C53 for ; Tue, 12 Jul 2022 10:25:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C8C583858C53 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-wm1-f52.google.com with SMTP id d13-20020a05600c34cd00b003a2dc1cf0b4so4516331wmq.4 for ; Tue, 12 Jul 2022 03:25:25 -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=MUY6h3kkCRnp9TUfocuFkGVX63HS0SJBtKTzo74YCsQ=; b=NYZgryt5gusblR17ZKE4J458QWeNF3Y0XB7zfPf/gCU2Umy/j92twTmDTDtXODhYiH KS7Fok9uLpDZVszYaGb40l6DTq1d/yBFusuEhWttcM37ZgpdC/jHviUgzHJJ9C+kMvay UsYSXqPMbJ2D9m2CtErcrjBt46+ZUbK4W0RWv87Z4vnIiekFJ8BhdLOZhC15rRg9sKFe SgUsQqMJ8YEHPk2EkYs3CZ6Rn+Kw0wERkSURCRacJ/xwaG+4t7HYZ7ECgHR6OaMa9AZo IJrWng1jrZf4r+5IQK8g+Hks3PMIFcaq0/N0HM8o4DGE23JFNmmunHDwBPGN+xN87Qc/ Wufg== X-Gm-Message-State: AJIora+KLCPJzGluBMwrEB8qu1Ka/AJLt46ze64stS+iblegavWm/eJs r4+iHRjT0NEBktAJ9Kxo9lmQLggu9ZQ= X-Google-Smtp-Source: AGRyM1tVsQFHuk85Ywz315SYkzT4bUArBJySInAjk5b5rp30IW0swoaeKHWKokEZerENZ/Uegmv68Q== X-Received: by 2002:a05:600c:a18c:b0:3a2:cbd5:f641 with SMTP id id12-20020a05600ca18c00b003a2cbd5f641mr2984755wmb.124.1657621523954; Tue, 12 Jul 2022 03:25:23 -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 24-20020a05600c22d800b003a0375c4f73sm9024030wmg.44.2022.07.12.03.25.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 12 Jul 2022 03:25:22 -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> <98108218-5cc6-fab8-fe17-319d37e8cb39@suse.de> From: Pedro Alves Message-ID: <53735898-5c00-1af6-c09a-7cc4622b64f7@palves.net> Date: Tue, 12 Jul 2022 11:25:21 +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: <98108218-5cc6-fab8-fe17-319d37e8cb39@suse.de> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, BODY_8BITS, 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 10:25:27 -0000 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.