From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x329.google.com (mail-wm1-x329.google.com [IPv6:2a00:1450:4864:20::329]) by sourceware.org (Postfix) with ESMTPS id 26537385803E for ; Wed, 10 Jan 2024 14:26:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 26537385803E Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 26537385803E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::329 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704896818; cv=none; b=snPT9tMZtNGOeEijxq4blCswqdD2IzAjGXL7H4a7xA5GOyXoS6YQP6rGl345TgPPpTq10V9VYghGGdDBVCu4xKGLTVCYz8nHrpSfkegqwe0Wo5IVEmDShag3mcsj2ClGhhCc40zF+M4UBHuRn2JOumhHg2How6Y2v44AvqsZRgQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704896818; c=relaxed/simple; bh=cvNc6bRbNIJtwCrtTPhThvnKdgkc2zRjgH54hzhB+Nw=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=Udq1zieDRBPoGGPVRDtpW1NOD2AATk4eSyk6iI5bG+A2H28G7QMnFUqks4liadJIYXj4DEic47C+AcpSxOhEAOnNoNMbpCudr4oQyiMo4b7KE0ZweFbRntBTmLfEyuX6gTmAigfYGMFj6RbS/vNvT+575yOfKhnN3qQpQjom7D0= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-x329.google.com with SMTP id 5b1f17b1804b1-40d6b4e2945so49202015e9.0 for ; Wed, 10 Jan 2024 06:26:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1704896814; x=1705501614; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:autocrypt:from:references:cc :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=e3rGfjfUEqqHaxcKmmtpDFwmsJ0eX8CTuCv4ozvBW7M=; b=cTIuQFn1Ozw7NH6sNFqxsPFbCPB+R4U0FtDo03hM1/Fus6kHKjFZdhpKr7wZ+xZHbS wzu4aoCz4zDcMa4a/+rOuQlPSzL7xK3Pw6NEreeZbyBdr27mQ2ZGEcRk27u07nrdJu3u UdV+X9jJYW8MPWd/0ol8y1D/Eao6cWVoTm8ZQFlXEnHuDtwSYhWv6sPhBPl4pOJJdVru dGOd6zda0avh+idnvNIO4DfVUth05hi7K3gO/OBjNl7o1qQtiIL1qtKNVP9DXE6bvxCG LAFW89p0RZ7zYit+F3FVX0QThGOADnIPjokxa3OM7bVJp9MvWJ1r0HvJD8a14GWBzVx3 udFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704896814; x=1705501614; h=content-transfer-encoding:in-reply-to:autocrypt:from:references:cc :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=e3rGfjfUEqqHaxcKmmtpDFwmsJ0eX8CTuCv4ozvBW7M=; b=fsz51jAtDHpufPhqXyFB1wZxLtes1UKQgpEEnY41jz0WxY9xZg0WdZ39qZBlVle4by qf3CbWxPtImbHw3APXqCYhoMED4k4nEslgbaUXrT+4q6CSToUIR13Sw2KXIcpLtPZR/Y QP7VSkvnrD4v89kl6zg+4B0Z0bC3UWWMbhXgT1Rmb0p9wskE8QIcaWnL669Th6xWTCBy nBOtjqYIvZ9N7BL5xodAnzguprfx0zMeYqk91sjpN5HCupYGn0HhEeUCwIxgFypPyFi8 6Es7DqkAmzJw1PyqHuUrXrbarODMbvKm9tyLZKClJSZJeXGUusMSSSCU0mG+0Rh6t+NZ sidQ== X-Gm-Message-State: AOJu0YyFHIO1oPFvX5rOvHJd//ZFlFIube9d3ZglVqaIAbUq5vxVVdtT 6WYctjlc2P1cXhuok1JGoAoXZmNGjw39 X-Google-Smtp-Source: AGHT+IFL7TCPX2WwsCXwP50r5as/hMX7ln89U56HQlPUE6BC7kPez/+cKdfTQejJjUhSpA0Ew71tdQ== X-Received: by 2002:a05:600c:4f0e:b0:40e:4a3d:83e with SMTP id l14-20020a05600c4f0e00b0040e4a3d083emr437731wmq.22.1704896813858; Wed, 10 Jan 2024 06:26:53 -0800 (PST) Received: from [10.156.60.236] (ip-037-024-206-209.um08.pools.vodafone-ip.de. [37.24.206.209]) by smtp.gmail.com with ESMTPSA id e30-20020a5d595e000000b0033776a50472sm4046555wri.10.2024.01.10.06.26.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 10 Jan 2024 06:26:53 -0800 (PST) Message-ID: <8803d3e2-688e-43ac-b710-5237fab8b054@suse.com> Date: Wed, 10 Jan 2024 15:26:53 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] bfd: make _bfd_section_size_insane part of the public API Content-Language: en-US To: Andrew Burgess Cc: binutils@sourceware.org References: <0c54069e-d907-4f03-8d7f-15374d4bfd6a@suse.com> <87frz58n7j.fsf@redhat.com> <877ckh8fjz.fsf@redhat.com> From: Jan Beulich Autocrypt: addr=jbeulich@suse.com; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL In-Reply-To: <877ckh8fjz.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3025.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,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 List-Id: On 10.01.2024 14:48, Andrew Burgess wrote: > Jan Beulich writes: >> On 10.01.2024 12:03, Andrew Burgess wrote: >>> Jan Beulich writes: >>>> On 06.12.2023 17:15, Andrew Burgess wrote: >>>>> If a BFD user is making use of a function like >>>>> bfd_get_section_contents to read a section into a pre-allocated >>>>> buffer, then that BFD user might also want to make use of >>>>> _bfd_section_size_insane prior to allocating the buffer they intend to >>>>> use in order to validate that the buffer size that plan to allocate is >>>>> sane. >>>>> >>>>> This commit makes _bfd_section_size_insane public, by renaming it to >>>>> bfd_section_size_insane. >>>>> >>>>> I've updated the existing uses within bfd/, I don't believe this >>>>> function is used outside of bfd/ currently. >>>>> >>>>> One place that I plan to make use of this function is in >>>>> gdb/gdb_bfd.c, in the function gdb_bfd_get_full_section_contents. >>>>> This change isn't included in this commit, but will come later if/when >>>>> this has been merged into bfd. >>>> >>>> Having seen your ping (and no other response), let me share my view: >>>> This function implements a certain policy, internal to the library. >>>> By exposing it, you would make external users dependent upon this >>>> specific policy. What if later we change our view on what's "insane"? >>> >>> I would expect and want external users to get the updated definition. >> >> And then break if we decide to lower the limit of "insane"? > > You've said this again later in your reply, and I think this is the bit > that I'm having trouble with. > > Why do you think things would break if the definition of "insane" > changed? > > Of course, I ask this under the assumption that there's no broken code > in the consumer, clearly: > > if (bfd_section_size_insane (...)) > // Broken code here... > else > // Working code here... > > Is going to break, and if *_insane() changes, then we're going to break > in more or less cases, but that seems slightly contrived. > > More likely we're going to write: > > if (bfd_section_size_insane (...)) > // Handle cases where the section is invalid. > else > // Load the section and process it. > > If the definition of *_insane() changes then I 100% want to enter the > error case more often. > > Of course, you might say, but what if the code was working fine before? > Then changing *_insane() is sending more cases down the error path... > > ... but I would suggest that this would be an indication of a bug being > introduced into *_insane(), I mean, if such a thing happened then we'd > already run into problems. > > So, in summary, the bit I'm not understanding here is why you feel > future changes to *_insane() would result in breakage in the consumer? Let's assume "insane" is anything above 4G right now (I don't recall what the real value is), and people are working with 3G objects, using any arbitrary consumer of libbfd. Then we choose lo lower the limit to 2G. While surely the limit is higher than the given example, the pattern remains: People working fine with the present limit might find themselves hosed if that limit was lowered. Hence why I dislike such heuristics, in turn meaning I would like to not see them put into wider use. >>> The function name of "insane" is a little unfortunate. I think if the >>> function had a better name then this change would seem far less >>> contentious. Consider a name of: >>> >>> validate_section_size_against_other_bfd_infernal_properties_of_the_elf_to_ensure_that_the_requested_size_is_likely_valid() >>> >>>> IOW external consumers want to implement their own, independent policy >>>> (if so desired). >>> >>> Sure, consumers _could_ implement their own policy, but IMHO, this would >>> be far worse than exposing the *_insane() function. >>> >>> What I (as a consumer) want is to check if the size that the BFD library >>> is reporting is valid or not. To do that I need to check details of the >>> ELF that I, as a BFD users, shouldn't have to bother with. (I thought) >>> the point of BFD was to abstract details of the file format. >> >> Well, your wording (correctly) makes an important distinction: "valid" != >> "sane". If this was a validity check, no question would arise about it >> being okay to expose. > > I didn't intend to make any such distinction. I was trying to say that > this function _is_ a validity check, it's just unfortunately named as a > sanity check. This is then where we disagree. There's no limit the ELF spec imposes on sh_size, just to take the most common (for GNU binutils) example. Hence there simply are no invalid size values (as far as ELF is concerned), and libbfd shouldn't make up any. > If I proposed renaming the function to 'is_bfd_section_size_valid()' > but left the implementation the same would you see a problem with this? Yes, because the size check failing _does not_ mean the size is invalid. Memory sizes grow all the time, storage sizes grow all the time, and object file as well as section sizes also (almost) only ever grow. > Would you object to making the *_valid() name public? If the function truly performed validity checks, of course I wouldn't object. Jan