From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 642673889816 for ; Fri, 25 Mar 2022 13:26:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 642673889816 Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-370-wUOBtMcgM3W-neNh8hx61w-1; Fri, 25 Mar 2022 09:26:40 -0400 X-MC-Unique: wUOBtMcgM3W-neNh8hx61w-1 Received: by mail-qt1-f199.google.com with SMTP id k1-20020ac85fc1000000b002e1c5930386so6067070qta.3 for ; Fri, 25 Mar 2022 06:26:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=3229UOmJe1nFLhMW/PA7TMJDYNApqd845VQxoMZoqxA=; b=D7LTHPiRf+PEagrGS3cb3dchIJsfwBCM3NLCRVHtPLzgqvmW7iLNBNsUVP7AOg8Bhv 1ZWTlgBMLvpZj9+FP2w6et59H2so+1nXDJznVOmy+ESngeaXmEmny91BlImDtf9cBhQI 0srjn0hvquE4yS7xNpBdlYhN8dxDudOtdT32hNyxtJRff1xxXK2OS12veWojt5+CxB+5 qFBli2a4ZiUXGPvdg3fopuSGaegExR2TUY5AZfzlTMWdQr3O2BFoQoKzl91nR/L5cQB2 +CbJbwdhAbg8cD6a9riJPN0uforxrhPPyOleJY+O2cB6hsS2XWKGCpue5KJ0JbdYdDBd FQ+A== X-Gm-Message-State: AOAM531t0U4XHnuI+hwkvytxLWArRczUXMEqoBFpjH4OeoZBc4B+LgsJ 4X4340Qh0cUyDJf8Q4OKB4VGMIQITy7pIBAkc77VHLUOsCcHWCqTcclkXXwK2i7HAmntZo4ueaf HdglR9PyIavigLkLUpw== X-Received: by 2002:a05:620a:1212:b0:67d:432b:7bbe with SMTP id u18-20020a05620a121200b0067d432b7bbemr6877846qkj.335.1648214800272; Fri, 25 Mar 2022 06:26:40 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxWeRQo30Qgz9+gsNgLU4iRSfK5Qi+PDcp2SI4wVZ6iJrfACSNrkbMJiSZyhWs1w5JWhywktA== X-Received: by 2002:a05:620a:1212:b0:67d:432b:7bbe with SMTP id u18-20020a05620a121200b0067d432b7bbemr6877811qkj.335.1648214799783; Fri, 25 Mar 2022 06:26:39 -0700 (PDT) Received: from [192.168.1.149] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id l8-20020a05622a174800b002e1e3f7d4easm5177282qtk.86.2022.03.25.06.26.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 25 Mar 2022 06:26:38 -0700 (PDT) Message-ID: <5db43fbd-2dad-af31-4565-76641be426b7@redhat.com> Date: Fri, 25 Mar 2022 09:26:36 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v2] middle-end/104854: Limit strncmp overread warnings To: Jonathan Wakely , Martin Sebor Cc: gcc-patches@gcc.gnu.org References: <20220311071236.2349381-1-siddhesh@gotplt.org> <20220315053158.1486555-1-siddhesh@gotplt.org> <01c49ecb-cd91-3bcc-c5a0-97e81281e1dd@gotplt.org> <4104ab01-c36c-12e9-5601-6d99019f6bef@redhat.com> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Mar 2022 13:26:44 -0000 On 3/17/22 06:35, Jonathan Wakely via Gcc-patches wrote: > On 15/03/22 14:36 -0600, Martin Sebor wrote: >> On 3/15/22 10:40, Siddhesh Poyarekar wrote: >>> On 15/03/2022 21:09, Martin Sebor wrote: >>>> The strncmp function takes arrays as arguments (not necessarily >>>> strings).? The main purpose of the -Wstringop-overread warning >>>> for calls to it is to detect calls where one of the arrays is >>>> not a nul-terminated string and the bound is larger than the size >>>> of the array.? For example: >>>> >>>> ?? char a[4], b[4]; >>>> >>>> ?? int f (void) >>>> ?? { >>>> ???? return strncmp (a, b, 8);?? // -Wstringop-overread >>>> ?? } >>>> >>>> Such a call is suspect: if one of the arrays isn't nul-terminated >>>> the call is undefined.? Otherwise, if both are nul-terminated there >>> >>> Isn't "suspect" too harsh a description though?? The bound does not >>> specify the size of a or b, it specifies the maximum extent to which to >>> compare a and b, the extent being any application-specific limit.? In >>> fact the limit could be the size of some arbitrary third buffer that the >>> contents of a or b must be copied to, truncating to the bound. >> >> The intended use of the strncmp bound is to limit the comparison to >> at most the size of the arrays or (in a subset of cases) the length >> of an initial substring. Providing an arbitrary bound that's not >> related to the sizes as you describe sounds very much like a misuse. >> >> As a historical note, strncmp was first introduced in UNIX v7 where >> its purpose, alongside strncpy, was to manipulate (potentially) >> unterminated character arrays like file names stored in fixed size >> arrays (typically 14 bytes).  Strncpy would fill the buffers with >> ASCII data up to their size and pad the rest with nuls only if there >> was room. >> >> Strncmp was then used to compare these potentially unterminated >> character arrays (e.g., archive headers in ld and ranlib).  The bound >> was the size of the fixed size array.  Its other use case was to compare >> leading portions of strings (e.g, when looking for an environment >> variable or when stripping "./" from path names). >> >> Since the early UNIX days, both strncpy and to a lesser extent strncmp >> have been widely misused and, along with many other functions in >> , a frequent source of bugs due to common misunderstanding >> of their intended purpose.  The aim of these warnings is to detect >> the common (and sometimes less common) misuses and bugs. >> >>> I agree the call is undefined if one of the arrays is not nul-terminated >>> and that's the thing; nothing about the bound is undefined in this >>> context, it's the NUL termination that is key. >>> >>>> is no point in calling strncmp with a bound greater than their sizes. >>> >>> There is, when the bound describes something else, e.g. the size of a >>> third destination buffer into which one of the input buffers may get >>> copied into.? Or when the bound describes the maximum length of a set of >>> strings where only a subset of the strings are reachable in the current >>> function and ranger sees it, allowing us to reduce our input string size >>> estimate.? The bounds being the maximum of the lengths of two input >>> strings is just one of many possibilities. >>> >>>> With no evidence that this warning is ever harmful I'd consider >>> >>> There is, the false positives were seen in Fedora/RHEL builds. >> >> I haven't seen these so I can't very well comment on them.  But I can >> assure you that warning for the code above is intentional.  Whether > > I don't think anybody is saying it wasn't intentional, the point is > that we can change our minds and do it differently based on feedback > and usage experience. > > If users no longer have faith in these warnings and just disable them > or ignore them, then the warnings do not find real bugs and are not > fit for purpose. > >> or not the arrays are nul-terminated, the expected way to call >> the function is with a bound no greater than their size (some coding >> guidelines are explicit about this; see for example the CERT C Secure >> Coding standard rule ARR38-C). > > That's fine. It shouldn't be in -Wall though. Perhaps a suitable compromise would be to add a separate warning flag specifically for the strn* warnings, so users deliberately using the bound to express a limit other than the length of the argument string (and confident that their strings are always NUL-terminated) can turn them off without turning off all the overread warnings. Jason