From patchwork Wed Nov 30 03:22:51 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 84938 Delivered-To: patch@linaro.org Received: by 10.140.20.101 with SMTP id 92csp62935qgi; Tue, 29 Nov 2016 19:23:40 -0800 (PST) X-Received: by 10.99.60.11 with SMTP id j11mr56439970pga.26.1480476220529; Tue, 29 Nov 2016 19:23:40 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id e13si33761903plj.182.2016.11.29.19.23.40 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 29 Nov 2016 19:23:40 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-442980-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-442980-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-442980-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=gmail.com DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=d9HB/bgppj+ocPMYw dM3X6M/taJ448sd9KXCogXRdb4dZTQPI7aTIIJBCJCR2MzGl90R/BLWnQv7rJtYa dHWjtrEuNxnGE6uHOnchjeQ4xmXLb4vYceTjHe1gun2ZMoLl+nhiOCoNFM0dV0ix ATVL0gYUwZuTjqhIg/QWW/l9ZA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=POU9ib+rr5r49j4xnc2DF0g qFHI=; b=BdYBww2s1fKvM1ufZsBcKu9BnsVcoZabrG+9YyYZjbS3wwsfaxc/R0F zDOrLPwlh1TXlaP18Z7gCJ+s4LrCVr/W40AFqK7vBBy/6svc6I6AcObJ7KhOB8DP ldKvZhDsB9sb63v954GJCt18vB5IJDcCtXZKQOXFFjUnGC53ENB0= Received: (qmail 109013 invoked by alias); 30 Nov 2016 03:23:22 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 108693 invoked by uid 89); 30 Nov 2016 03:22:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=LINE, sk:tree-ob, UD:tree-object-size.c, sk:treeob X-HELO: mail-qt0-f177.google.com Received: from mail-qt0-f177.google.com (HELO mail-qt0-f177.google.com) (209.85.216.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 30 Nov 2016 03:22:56 +0000 Received: by mail-qt0-f177.google.com with SMTP id w33so175868529qtc.3 for ; Tue, 29 Nov 2016 19:22:56 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to; bh=jBHJudKcAjNd8NYEu835rgU6D5UEsoBHjYROncJVVVQ=; b=e3dWfK5YZd1j/XCUp9O62jWTfQh6ZXiMxySWS+TWuNIa2DHw1g6rkGjC5pekthwpa3 f4mz5cshygkfU9pkND/X1evjxqj3p9oPvt/pWENvmkfRemTTg3fOBHYLjP33ybB82Pmr C7IjcTcM5F5kZXfok7cSbw4wozZfBOmoves88GHr5q+I90RBufUDr6DSiStndLjSeGju wWTu4K6FsptnvN5tL8nHReoAvtJDHqNSOBwYmJXcw+IUsyqT88SuscdaDfW2QFc+9tQT C9U6nGyflhoZT2DMXYNWchFUR/T6+/nn6FE/wClGhoLlLFxE6/lLpC/klbu92RPL/i22 hbhg== X-Gm-Message-State: AKaTC02y0N89JfBAdqs0WucqtUSFGmEMqASORlzm7hQJK9ncdQD/wamBwhI9y9GiGMnMVA== X-Received: by 10.200.38.177 with SMTP id 46mr10689455qto.107.1480476174915; Tue, 29 Nov 2016 19:22:54 -0800 (PST) Received: from [192.168.0.26] (75-166-206-79.hlrn.qwest.net. [75.166.206.79]) by smtp.gmail.com with ESMTPSA id l37sm24310112qkh.15.2016.11.29.19.22.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 29 Nov 2016 19:22:54 -0800 (PST) Subject: Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245) To: Jeff Law , Gcc Patch List References: <8000f699-dee9-29be-d887-68b57ff0fafc@gmail.com> <2fa72d08-0fac-28b3-4fcf-c70f83b18c8d@gmail.com> <41a29676-9e11-cf26-c9b1-12ae255194e3@gmail.com> <12efe89c-dbad-2f77-0e26-299a4a6ccde8@redhat.com> <43cf56f0-49af-e6ba-7e4c-b78ed67c07c1@gmail.com> From: Martin Sebor Message-ID: <3ce498e7-3c3f-600d-2f2b-cfacea646140@gmail.com> Date: Tue, 29 Nov 2016 20:22:51 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <43cf56f0-49af-e6ba-7e4c-b78ed67c07c1@gmail.com> X-IsSubscribed: yes >>> That said, I defer to you on how to proceed here. I'm prepared >>> to do the work(*) but I do worry about jeopardizing the chances >>> of this patch and the others making it into 7.0. >> So would it make sense to just init/fini the b_o_s framework in your >> pass and for builtin expansion? > > I think that should work for the sprintf checking. Let me test it. > We can deal with the memxxx and strxxx patch (53562) independently > if you prefer. Attached is a modified patch that calls {init,fini}_object_sizes() from the gimple-ssa-sprintf pass instead. While this works fine, I do like the approach of making the calls in a single function better because it makes for a more robust API. Decoupling the init/fini calls from the compute_object_size() function that depends on them having been made makes the API easier to accidentally misuse by calling one while forgetting to call one or both of the other two. Martin PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer gcc/testsuite/ChangeLog: PR middle-end/78245 * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests. gcc/ChangeLog: PR middle-end/78245 * gimple-ssa-sprintf.c (get_destination_size): Call compute_object_size. * tree-object-size.c (addr_object_size): Adjust. (pass_through_call): Adjust. (internal_object_size): New function. (compute_builtin_object_size): Call internal_object_size. (pass_object_sizes::execute): Adjust. * tree-object-size.h (fini_object_sizes): Declare. diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c index ead8b0e..34b3723 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -2466,6 +2466,9 @@ get_destination_size (tree dest) a member array as opposed to the whole enclosing object), otherwise use type-zero object size to determine the size of the enclosing object (the function fails without optimization in this type). */ + + init_object_sizes (); + int ost = optimize > 0; unsigned HOST_WIDE_INT size; if (compute_builtin_object_size (dest, ost, &size)) @@ -2800,6 +2803,8 @@ pass_sprintf_length::execute (function *fun) } } + fini_object_sizes (); + return 0; } diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c index 8d97fa8..9874332 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c @@ -1,5 +1,10 @@ /* { dg-do compile } */ -/* { dg-options "-std=c99 -O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */ +/* { dg-options "-O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */ +/* Verify that all sprintf built-ins detect overflow involving directives + with non-constant arguments known to be constrained by some range of + values, and even when writing into dynamically allocated buffers. + -O2 (-ftree-vrp) is necessary for the tests involving ranges to pass, + otherwise -O1 is sufficient. */ #ifndef LINE # define LINE 0 @@ -7,18 +12,26 @@ #define bos(x) __builtin_object_size (x, 0) -#define T(bufsize, fmt, ...) \ - do { \ - if (!LINE || __LINE__ == LINE) \ - { \ - char *d = (char *)__builtin_malloc (bufsize); \ - __builtin___sprintf_chk (d, 0, bos (d), fmt, __VA_ARGS__); \ - sink (d); \ - } \ - } while (0) +/* Defined (and redefined) to the allocation function to use, either + malloc, or alloca, or a VLA. */ +#define ALLOC(p, n) (p) = __builtin_malloc (n) -void -sink (void*); +/* Defined (and redefined) to the sprintf function to exercise. */ +#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...) \ + __builtin___sprintf_chk (d, 0, objsize, fmt, __VA_ARGS__) + +#define T(bufsize, fmt, ...) \ + do { \ + if (!LINE || __LINE__ == LINE) \ + { \ + char *d; \ + ALLOC (d, bufsize); \ + TEST_SPRINTF (d, 0, bos (d), fmt, __VA_ARGS__); \ + sink (d); \ + } \ + } while (0) + +void sink (void*); /* Identity function to verify that the checker figures out the value of the operand even when it's not constant (i.e., makes use of @@ -232,3 +245,88 @@ void test_sprintf_chk_range_sshort (signed short *a, signed short *b) T ( 4, "%i", Ra (998, 999)); T ( 4, "%i", Ra (999, 1000)); /* { dg-warning "may write a terminating nul past the end of the destination" } */ } + +/* Exercise ordinary sprintf with malloc. */ +#undef TEST_SPRINTF +#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...) \ + __builtin_sprintf (d, fmt, __VA_ARGS__) + +void test_sprintf_malloc (const char *s, const char *t) +{ +#define x x () + + T (1, "%-s", x ? "" : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : ""); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : s); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : t); + + T (2, "%-s", x ? "" : "1"); + T (2, "%-s", x ? "" : s); + T (2, "%-s", x ? "1" : ""); + T (2, "%-s", x ? s : ""); + T (2, "%-s", x ? "1" : "2"); + T (2, "%-s", x ? "" : "12"); /* { dg-warning "nul past the end" } */ + T (2, "%-s", x ? "12" : ""); /* { dg-warning "nul past the end" } */ + + T (2, "%-s", x ? "" : "123"); /* { dg-warning "into a region" } */ + T (2, "%-s", x ? "123" : ""); /* { dg-warning "into a region" } */ + +#undef x +} + +/* Exercise ordinary sprintf with alloca. */ +#undef ALLOC +#define ALLOC(p, n) (p) = __builtin_alloca (n) + +void test_sprintf_alloca (const char *s, const char *t) +{ +#define x x () + + T (1, "%-s", x ? "" : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : ""); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : s); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : t); + + T (2, "%-s", x ? "" : "1"); + T (2, "%-s", x ? "" : s); + T (2, "%-s", x ? "1" : ""); + T (2, "%-s", x ? s : ""); + T (2, "%-s", x ? "1" : "2"); + T (2, "%-s", x ? "" : "12"); /* { dg-warning "nul past the end" } */ + T (2, "%-s", x ? "12" : ""); /* { dg-warning "nul past the end" } */ + + T (2, "%-s", x ? "" : "123"); /* { dg-warning "into a region" } */ + T (2, "%-s", x ? "123" : ""); /* { dg-warning "into a region" } */ + +#undef x +} + +/* Exercise ordinary sprintf with a VLA. */ +#undef ALLOC +#define ALLOC(p, n) char vla [i (n)]; (p) = vla + +void test_sprintf_vla (const char *s, const char *t) +{ +#define x x () + + T (1, "%-s", x ? "" : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : ""); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : s); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : t); + + T (2, "%-s", x ? "" : "1"); + T (2, "%-s", x ? "" : s); + T (2, "%-s", x ? "1" : ""); + T (2, "%-s", x ? s : ""); + T (2, "%-s", x ? "1" : "2"); + T (2, "%-s", x ? "" : "12"); /* { dg-warning "nul past the end" } */ + T (2, "%-s", x ? "12" : ""); /* { dg-warning "nul past the end" } */ + + T (2, "%-s", x ? "" : "123"); /* { dg-warning "into a region" } */ + T (2, "%-s", x ? "123" : ""); /* { dg-warning "into a region" } */ + +#undef x +} diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c index 1317ad7..3a2d54d 100644 --- a/gcc/tree-object-size.c +++ b/gcc/tree-object-size.c @@ -50,6 +50,7 @@ static const unsigned HOST_WIDE_INT unknown[4] = { 0 }; +static bool internal_object_size (tree, int, unsigned HOST_WIDE_INT *); static tree compute_object_offset (const_tree, const_tree); static bool addr_object_size (struct object_size_info *, const_tree, int, unsigned HOST_WIDE_INT *); @@ -187,7 +188,7 @@ addr_object_size (struct object_size_info *osi, const_tree ptr, if (!osi || (object_size_type & 1) != 0 || TREE_CODE (TREE_OPERAND (pt_var, 0)) != SSA_NAME) { - compute_builtin_object_size (TREE_OPERAND (pt_var, 0), + internal_object_size (TREE_OPERAND (pt_var, 0), object_size_type & ~1, &sz); } else @@ -491,14 +492,14 @@ pass_through_call (const gcall *call) } -/* Compute __builtin_object_size value for PTR and set *PSIZE to - the resulting value. OBJECT_SIZE_TYPE is the second argument - to __builtin_object_size. Return true on success and false - when the object size could not be determined. */ +/* Compute the size of the object pointed to by PTR and set *PSIZE + to the resulting value. OBJECT_SIZE_TYPE is the second argument + to __builtin_object_size. Return true on success and false when + the object size could not be determined. */ bool -compute_builtin_object_size (tree ptr, int object_size_type, - unsigned HOST_WIDE_INT *psize) +internal_object_size (tree ptr, int object_size_type, + unsigned HOST_WIDE_INT *psize) { gcc_assert (object_size_type >= 0 && object_size_type <= 3); @@ -534,7 +535,7 @@ compute_builtin_object_size (tree ptr, int object_size_type, ptr = gimple_assign_rhs1 (def); if (cst_and_fits_in_hwi (offset) - && compute_builtin_object_size (ptr, object_size_type, psize)) + && internal_object_size (ptr, object_size_type, psize)) { /* Return zero when the offset is out of bounds. */ unsigned HOST_WIDE_INT off = tree_to_shwi (offset); @@ -664,6 +665,18 @@ compute_builtin_object_size (tree ptr, int object_size_type, return *psize != unknown[object_size_type]; } +/* Compute __builtin_object_size value for PTR and set *PSIZE to + the resulting value. OBJECT_SIZE_TYPE is the second argument + to __builtin_object_size. Return true on success and false + when the object size could not be determined. */ + +bool +compute_builtin_object_size (tree ptr, int object_size_type, + unsigned HOST_WIDE_INT *psize) +{ + return internal_object_size (ptr, object_size_type, psize); +} + /* Compute object_sizes for PTR, defined to VALUE, which is not an SSA_NAME. */ static void @@ -1230,7 +1243,7 @@ init_object_sizes (void) /* Destroy data structures after the object size computation. */ -static void +void fini_object_sizes (void) { int object_size_type; @@ -1325,7 +1338,7 @@ pass_object_sizes::execute (function *fun) { tree type = TREE_TYPE (lhs); unsigned HOST_WIDE_INT bytes; - if (compute_builtin_object_size (ptr, object_size_type, + if (internal_object_size (ptr, object_size_type, &bytes) && wi::fits_to_tree_p (bytes, type)) { diff --git a/gcc/tree-object-size.h b/gcc/tree-object-size.h index 38c3e07..b656339 100644 --- a/gcc/tree-object-size.h +++ b/gcc/tree-object-size.h @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see #define GCC_TREE_OBJECT_SIZE_H extern void init_object_sizes (void); +extern void fini_object_sizes (void); extern bool compute_builtin_object_size (tree, int, unsigned HOST_WIDE_INT *); #endif // GCC_TREE_OBJECT_SIZE_H