From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68535 invoked by alias); 27 Apr 2017 13:58:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 67596 invoked by uid 89); 27 Apr 2017 13:58:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=POD, it'll, itll X-HELO: mail-wm0-f43.google.com Received: from mail-wm0-f43.google.com (HELO mail-wm0-f43.google.com) (74.125.82.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 27 Apr 2017 13:58:38 +0000 Received: by mail-wm0-f43.google.com with SMTP id r190so19431411wme.1 for ; Thu, 27 Apr 2017 06:58:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=Cj7K7jjTusTzBNJjvj2UQPnstYWCG0hv89EweDmkz5I=; b=FslR3RIT7WJXYNgiwff8ku9pWEneDaU6g8y4pJq4wmBc65Osv9w5/XxUHie+eqZaF4 FrpsNfGPOv2uW7k0fpdM1WS2VtGoMba3nfHz1Y9YWmvycpoDFwnM/l10NfCjjC9TUaA1 Q9lWACP49wWDBygLE5VD3jwn0GRPlv6yYe490nbvjN33c/SQTnNtWrCvzjXUin2PBzhI EaCOQUr1dcmxwv2vXy6kYInk0tpUN5Wwn99kK1U3eYMwt7DfZ/8wUvJZqujWCgPnWK2l OqohDfDg0qZOiir4ooKgG69jRsr5tjsbXPi+cu3bNRYriMG2ycWFG0kls4XPL61wsTJH M5Nw== X-Gm-Message-State: AN3rC/7uaPuI1dlzTRzClt05ZeSLTiEC7i7EIXoUuI4LPd+fnx5Ymx0q 1M9HXxVMZPTnBCJ+ X-Received: by 10.28.129.65 with SMTP id c62mr2303712wmd.79.1493301518924; Thu, 27 Apr 2017 06:58:38 -0700 (PDT) Received: from [192.168.0.102] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id u76sm4181187wrb.27.2017.04.27.06.58.37 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 27 Apr 2017 06:58:38 -0700 (PDT) Subject: Re: [PATCH 1/5] Poison non-POD memset & non-trivially-copyable memcpy/memmove To: Simon Marchi References: <1492050475-9238-1-git-send-email-palves@redhat.com> <1492050475-9238-2-git-send-email-palves@redhat.com> <9ee5551a7999a72a0040f15e6e5410a1@polymtl.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Thu, 27 Apr 2017 13:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <9ee5551a7999a72a0040f15e6e5410a1@polymtl.ca> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-04/txt/msg00735.txt.bz2 On 04/24/2017 02:53 AM, Simon Marchi wrote: > On 2017-04-23 21:12, Simon Marchi wrote: >> On 2017-04-12 22:27, Pedro Alves wrote: >>> This patch catches invalid initialization of non-POD types with >>> memset, at compile time. >> >> Would it be possible to do something similar but to catch uses of >> XNEW/XCNEW with types that need new? XNEW is defined as: >> >> #define XNEW(T) ((T *) xmalloc (sizeof (T))) >> >> I just tried this, and it seems to work well: >> >> #define assert_pod(T) static_assert(std::is_pod::value) >> >> #undef XNEW >> #define XNEW(T) ({ assert_pod(T); (T *) xmalloc (sizeof (T)); }) >> #undef XCNEW >> #define XCNEW(T) ({ assert_pod(T); (T *) xcalloc (1, sizeof (T)); }) >> >> assuming the compiler knows about statement expressions. > > Actually, it should probably use std::is_trivially_constructible. > And I > suppose we could do the same with xfree, delete it when > !std::is_trivially_destructible. I think you wanted std::is_trivially_default_constructible for XNEW. I think that we want _both_ conditions (*constructible and *destructible) on both XNEW and xfree. For example, it'll be good to catch the mismatching new/delete that could sneak in otherwise: // type with trivial constructor struct A { // A() = default; ~A() { /* do something with side effects */ } // not trivial }; // type with trivial destructor struct B { B() { /* do something with side effects */ } // not trivial //~B() = default; }; void foo () { A *a = XNEW (struct A); delete a; B *b = new B; xfree (b); } Calling delete on a pointer not allocated with new is undefined behavior. These mismatches are also flagged by -fsanitize=address, but making them compile-time errors would be even better. This wouldn't catch allocating types that are both trivially default constructible and trivially destructible, and which _also_ have non-default ctors, like this, for example: struct C { C() = default; explicit C(int) { /* some side effects */ } }; static_assert (std::is_trivially_default_constructible::value, ""); static_assert (std::is_trivially_destructible::value, ""); C *b = new C(1); xfree (b); // whoops, technically undefined. -fsanitify=address likely complains. but std::is_pod wouldn't either. If we make a type non-standard-layout, then it no longer is POD: struct D { // Mix of public/private fields => not POD public: int a; private: int b; }; This (D) case is likely to not really be problematic in practice WRT to allocation/deallocation with malloc/free, but it still feels like a code smell to me. I'd be willing to try forcing use of new/delete for these types too. This would suggest using the bigger std::is_pod hammer in XNEW/xfree instead of just std::is_trivially_*ctible. But I'd understand if others disagree. Thanks, Pedro Alves