From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61134 invoked by alias); 18 Jan 2017 17:45:32 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 61081 invoked by uid 89); 18 Jan 2017 17:45:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=sf, 6700, 392, sk:_M_defa X-HELO: mail-qt0-f196.google.com Received: from mail-qt0-f196.google.com (HELO mail-qt0-f196.google.com) (209.85.216.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Jan 2017 17:45:28 +0000 Received: by mail-qt0-f196.google.com with SMTP id l7so2772283qtd.3 for ; Wed, 18 Jan 2017 09:45:28 -0800 (PST) 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=Oxyx2E8zMd0N5RqsdeLzOO9q/wwr+FsZiqgOUxRclVM=; b=XIuv3ZpYz4raKLkrqV9II5jr61hhGmp2VoyaHu26/pT6b9/ZaEfoznFHwD/Cep8+NW GLCI5Ep4D3oIAwQdKp/6wS5S780ZJD7I8JX7+jXBASl2cZDGNZxb+iLGXIu1YH1V29I+ NzyBPnFW7dqEIWpX7J9L62akkOCnoDa38fOLfflIdha5gA6p/x1wncFIi6w9Oiw73dou tm7GmvIHGi8mSUD0ziYD+hwZh+9aBKfM1OgU10PUr0i1AOyHQtN7aFU5UZ6d5/k7R8hj RgGD6K0PQaqeNdLCWMdRZCGYFWv8yHi/9crZD2MyCHR37GCu2WBUDAbHFyx+EYByAdxw pJqg== X-Gm-Message-State: AIkVDXL0T0GjpgQ5388wtSRAuV8xvXAmmI20Wl0vY64kURWmSUzeFkkdYLk/K//96bu9ug== X-Received: by 10.55.122.130 with SMTP id v124mr3806055qkc.19.1484761526782; Wed, 18 Jan 2017 09:45:26 -0800 (PST) Received: from [192.168.0.3] (97-118-114-123.hlrn.qwest.net. [97.118.114.123]) by smtp.gmail.com with ESMTPSA id 37sm747914qto.43.2017.01.18.09.45.25 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 Jan 2017 09:45:26 -0800 (PST) Subject: Re: [PATCH] avoid calling memset et al. with excessively large sizes (PR 79095) To: Jakub Jelinek , Jeff Law References: <497e7848-5690-2c4e-7277-cab674a60a35@gmail.com> <20170117073804.GU1867@tucnak> <49d6004c-6a5d-0ef3-cf15-a39016b09847@gmail.com> <0ec47b4d-c5bf-d967-a103-1ff73dbda1ec@redhat.com> <20170118081059.GI1867@tucnak> Cc: Gcc Patch List From: Martin Sebor Message-ID: Date: Wed, 18 Jan 2017 18:08:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170118081059.GI1867@tucnak> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg01377.txt.bz2 On 01/18/2017 01:10 AM, Jakub Jelinek wrote: > On Tue, Jan 17, 2017 at 10:59:43PM -0700, Jeff Law wrote: >>> I agree that breaking those applications would be bad. It could >>> be dealt with by adding an option to let them disable the insertion >>> of the trap. With the warning, programmers would get a heads up >>> that their (already dubious) code won't work otherwise. I don't >>> think it's a necessary or wise to have the default mode be the most >>> permissive (and most dangerous) and expect users to tweak options >>> to make it safe. Rather, I would argue that it should be the other >>> way around. Make the default safe and strict and let the advanced >>> users who know how deal with the risks tweak those options. >> I still come back to the assertion that changing this loop to a mem* is >> fundamentally the wrong thing to do as it changes something that has well >> defined semantics to something that is invalid. >> >> Thus the transformation into a mem* call is invalid. > > The mem* call is as valid as the loop, it will work exactly the same. The problem here isn't the loop itself or the memset call but the bounds computed from the inputs. In the test case submitted with the bug the inputs supplied by the program are well within a valid range, yet the bounds of the loop that are computed by GCC from those inputs are excessive because they are based on impossible ranges (or an incomplete view of the program). There is no unbounded loop in void f(std::vector &v) { size_t n = v.size (); if (n > 1 && n < 5) v.resize (n - 1); } yet GCC synthesizes one: void f(std::vector&) (struct vector & v) { ... [100.00%]: _7 = MEM[(int * *)v_5(D)]; // v._M_impl._M_start _8 = MEM[(int * *)v_5(D) + 8B]; // v._M_impl._M_finish _9 = (long int) _8; _10 = (long int) _7; _11 = _9 - _10; _12 = _11 /[ex] 4; _13 = (long unsigned int) _12; // v.size() _1 = _13 + 18446744073709551614; // v.size() - 4 if (_1 <= 2) // if (v.size() <= 6) goto ; [36.64%] else goto ; [63.36%] [36.64%]: _2 = _13 + 18446744073709551615; // v.size() - 1 if (_2 > _13) // if (v.size() == 0) goto ; [29.56%] // this can't happen else goto ; [70.44%] [5.85%]: _24 = v_5(D)->D.15828._M_impl._M_end_of_storage; _25 = (long int) _24; _28 = _25 - _9; // bytes left if (_28 == -4) // this can't happen goto ; [67.00%] else goto ; [33.00%] [3.92%]: __builtin_memset (_8, 0, 18446744073709551612); // (size_t)-4 ... [1.93%]: std::__throw_length_error ("vector::_M_default_append"); > If you have say on 32-bit target > #include > > int > main () > { > char *p = malloc (3U * 1024 * 1024 * 1024); > if (p == NULL) > return 0; > size_t i; > for (i = 0; i < 3U * 1024 * 1024 * 1024; i++) > p[i] = 6; > use (p); > return 0; > } Unlike in the test case submitted with the bug, here the loop is in the source and warning on it would be justified for most programs. Here's a test case that's closer to the one from the bug. It also ends up with the out of bounds memset even at -O1, during PRE. typedef __SIZE_TYPE__ size_t; struct S int *p0, *p1, *p2; size_t size () const { return p1 - p0; } void f (size_t n) { if (n > size ()) // can't happen because foo (n - size ()); // n is in [1, MIN(size() - 1, 3)] else if (n < size ()) bar (p0 + n); } void foo (size_t n) { size_t left = (size_t)(p2 - p1); if (left >= n) __builtin_memset (p2, 0, n * sizeof *p2); } void bar (int*); }; void f (S &s) { size_t n = s.size (); if (n > 1 && n < 5) s.f (n - 1); } Martin