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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 0D8533858420 for ; Fri, 15 Oct 2021 08:47:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0D8533858420 Received: from mail-vk1-f197.google.com (mail-vk1-f197.google.com [209.85.221.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-373-osDYDAJkMSmu1phO2OYTWA-1; Fri, 15 Oct 2021 04:47:12 -0400 X-MC-Unique: osDYDAJkMSmu1phO2OYTWA-1 Received: by mail-vk1-f197.google.com with SMTP id 46-20020a056122072e00b002a484c884c9so1636706vki.17 for ; Fri, 15 Oct 2021 01:47: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:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=w/iV58wDh9SjHBNZcZgDvEHWI2GBCv50UtVS870EzYI=; b=QQg0HoefoiLkklD0q3s50z0rSFbjn8DB4NYtmH9zNXqyK4lpTg+sOX/Ymv31WtPFFS gvgrp8418Z2n3aVOgvnUQLnK58J9p4IY2B83ePw8qV0R6QLNEAuR2uxM9AS02iYCNU3b KFd26L00uhkksDF0unr21ilvbdF5wWx8/6O4eeHs7wQuHmAvZV4/5/z7w0yaIQ5BpQFn +1/IdlgbUfN4IWFPBf7Q2U4vaWtOsEecbLgbqp+lf4hYyYetGaBk8yO8yEbRJwlZJ+JM pxySXa4wFj3jaMZGFL+0v705WUalU7FRIuABIa8dKeA3l7HETOupIxj1ARJx896SmV5R wr3w== X-Gm-Message-State: AOAM531eawIIpUkhjU+cSsO6mE/HfpQsQvrw0zbxcUoWDMCCE6h737+9 wLPcCUHDexx+jcPSBn58p9Jqfqr9UsABJuwyTc1dTqIsVxNyKauW05qh0DPSOOBCboGnyR26nx3 Y2CWCgLxN4vZrWtZO4wh4s9hnYRi1uVjmQQ== X-Received: by 2002:ab0:338e:: with SMTP id y14mr11625360uap.70.1634287631734; Fri, 15 Oct 2021 01:47:11 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzkM2TBIdziLHs01YgihbY0j8xqNDcrKvlF3dkrrpvqjvJxtfqR8M9MF6rUQwkYM/9LipNpuFvn6yfoI2jtVjo= X-Received: by 2002:ab0:338e:: with SMTP id y14mr11625348uap.70.1634287631488; Fri, 15 Oct 2021 01:47:11 -0700 (PDT) MIME-Version: 1.0 References: <2f87571d-8f17-6f70-d163-a9a38a2c37db@gmail.com> <2225a991-23e5-ea1e-39d6-f2f007f4fed5@gmail.com> In-Reply-To: <2225a991-23e5-ea1e-39d6-f2f007f4fed5@gmail.com> From: Jonathan Wakely Date: Fri, 15 Oct 2021 09:47:00 +0100 Message-ID: Subject: Re: [PATCH] libstdc++: Check [ptr, end) and [ptr, ptr+n) ranges with _GLIBCXX_ASSERTIONS To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: "libstdc++" , gcc Patches X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=unavailable 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, 15 Oct 2021 08:47:17 -0000 On Fri, 15 Oct 2021 at 06:19, Fran=C3=A7ois Dumont wrote: > > On 14/10/21 7:43 pm, Jonathan Wakely wrote: > > On Thu, 14 Oct 2021 at 18:11, Fran=C3=A7ois Dumont wrote: > >> Hi > >> > >> On a related subject I am waiting for some feedback on: > >> > >> https://gcc.gnu.org/pipermail/libstdc++/2021-August/053005.html > > I'm concerned that this adds too much overhead for the > > _GLIBCXX_ASSERTIONS case. It adds function calls which are not > > necessarily inlined, and which perform arithmetic and comparisons on > > the arguments. That has a runtime cost which is non-zero. > > I thought that limiting the checks to __valid_range would be fine for > _GLIBCXX_ASSERTIONS. If you do not want any overhead you just don't > define it. Then you get no checks at all. The point of _GLIBCXX_ASSERTIONS is to get *some* checking, without too much overhead. If you are willing to accept the overhead we already have _GLIBCXX_DEBUG for that. We could consider a second level of _GLIBCXX_ASSERTIONS=3D2 that turns on extra checks, but we need to be careful about adding any non-trivial checks to _GLIBCXX_ASSERTIONS=3D1 (which is what is used today in major linux distributions, to build every C++ program and library in the OS). > > > > > The patches I sent in this thread have zero runtime cost, because they > > use the compiler built-in which compiles away to nothing if the sizes > > aren't known. > I'll try to find out if it can help for the test case on std::copy which > I was adding with my proposal. > > > >> On 11/10/21 6:49 pm, Jonathan Wakely wrote: > >>> This enables lightweight checks for the __glibcxx_requires_valid_rang= e > >>> and __glibcxx_requires_string_len macros when _GLIBCXX_ASSERTIONS is > >>> defined. By using __builtin_object_size we can check whether the end= of > >>> the range is part of the same object as the start of the range, and > >>> detect problems like in PR 89927. > >>> > >>> libstdc++-v3/ChangeLog: > >>> > >>> * include/debug/debug.h (__valid_range_p, __valid_range_n): Ne= w > >>> inline functions using __builtin_object_size to check ranges > >>> delimited by pointers. > >>> [_GLIBCXX_ASSERTIONS] (__glibcxx_requires_valid_range): Use > >>> __valid_range_p. > >>> [_GLIBCXX_ASSERTIONS] (__glibcxx_requires_string_len): Use > >>> __valid_range_n. > >>> > >>> > >>> The first patch allows us to detect bugs like string("foo", "bar"), > >>> like in PR 89927. Debug mode cannot currently detect this. The new > >>> check uses the compiler built-in to detect when the two arguments are > >>> not part of the same object. This assumes we're optimizing and the > >>> compiler knows the values of the pointers. If it doesn't, then the > >>> function just returns true and should inline to nothing. > >> I see, it does not detect that input pointers are unrelated but as the= y > >> are the computed size is >=3D __sz. > >> > >> Isn't it UB to compare unrelated pointers ? > > Yes, and my patch doesn't compare any pointers, does it? > > > + __UINTPTR_TYPE__ __f =3D (__UINTPTR_TYPE__)__first; > + __UINTPTR_TYPE__ __l =3D (__UINTPTR_TYPE__)__last; > + if (const std::size_t __sz =3D __builtin_object_size(__first, 3)) > + return __f <=3D __l && (__l - __f) <=3D __sz; > > Isn't it a comparison ? It's not comparing pointers, it's comparing integers. To avoid the unspecified behaviour of comparing unrelated pointers. > > But maybe this is what the previous cast is for, I never understood it. > > Note that those cast could be moved within the if branch, even if I > guess that the compiler does it. At -O1 the casts are zero cost, they don't generate any code. At -O0, you have so much overhead for every line of code that this doesn't make much difference! But yes, we could move them into the if statement.