From patchwork Wed Nov 9 00:09:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 81417 Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp1823333qge; Tue, 8 Nov 2016 16:10:15 -0800 (PST) X-Received: by 10.99.44.84 with SMTP id s81mr22567089pgs.153.1478650215804; Tue, 08 Nov 2016 16:10:15 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id a203si11914324pfa.99.2016.11.08.16.10.15 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 08 Nov 2016 16:10:15 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-440793-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-440793-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-440793-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:from :subject:to:message-id:date:mime-version:content-type; q=dns; s= default; b=jDKhyPM5Wk00mi2NoM4EVjAbAwjmveoV7F3xSepipw4/AHwHHQI+v umN3hLJxxQ0162gd9SJo84kLos/mBVRpQvaOdcnCM8XeKgcr6h9ODj3Mn18zaub/ 5xBRVuOrNUpGjbW8tS+xJTu8Jlj/uRsjq40ZAunxTgYmUk1me50rl0= 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:from :subject:to:message-id:date:mime-version:content-type; s= default; bh=vfS17FpFw0Bz6j0ChUBzaRylVgc=; b=Hi73kCix2cIYoLjUgHDE I/NRjcUgQTJwCDpScyzoqdzpfw5c0jTrH3bjwhkvz/9dMUBnAkPpNOx5w+LXZhtn bp8oxF58pH43r/O4jRnvQBSK9kEDAzQjGPoY9SrsYKpmOtqh1TRL1FeSzhcVHj1Y +01W0qACYiMfh+GcG7yqGKc= Received: (qmail 44371 invoked by alias); 9 Nov 2016 00:09:56 -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 44357 invoked by uid 89); 9 Nov 2016 00:09:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.4 required=5.0 tests=AWL, BAYES_20, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=Identity, figures, 6646, 1877 X-HELO: mail-qk0-f180.google.com Received: from mail-qk0-f180.google.com (HELO mail-qk0-f180.google.com) (209.85.220.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 09 Nov 2016 00:09:53 +0000 Received: by mail-qk0-f180.google.com with SMTP id n21so127142710qka.3 for ; Tue, 08 Nov 2016 16:09:53 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:subject:to:message-id:date:user-agent :mime-version; bh=rIKxsI237Xi8LK60WDrspPPnRFQWuxZtwKwNeiZw7HM=; b=ijCYzKzRBUCYSZIkVEI7tH/i/zMkV9iTt8bd2kT6RbSlkLSNm5i8Dlt8jYXWrQwBNN YE9raVOZPMk4+a2N8KiKqFLdxCbYSQI2tdl6l9bVoWOOgg24kEBF0+DzW+GXfVaGKYva ZvLWal3VOmCib3Yx2B8GhQjusv8j4EgyOMIynVihBHYUzyxtTWxQwJysuzSvDxk3FuEw /mYkEPChICw74IEUcdJgUuycrfCJDaCToKoPuZPlRCA87ENtWOJAjWqv/pUx08J8AxU5 D2TaTfvvD2AhBM6EYM6waYSXZDk1jR64lPZfkZYeuBLRPtQee7fZ/iDlU/lvWft8+3u8 e5UA== X-Gm-Message-State: ABUngvdAjO/YN84/L60GPcwSJUm/1icP8r+FQpI+LalJTEGmXf6Ok4cmOcJTfpHMVsXMYw== X-Received: by 10.55.19.144 with SMTP id 16mr15732119qkt.22.1478650191663; Tue, 08 Nov 2016 16:09:51 -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 u44sm20827870qtu.5.2016.11.08.16.09.50 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 08 Nov 2016 16:09:51 -0800 (PST) From: Martin Sebor Subject: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245) To: Gcc Patch List Message-ID: <8000f699-dee9-29be-d887-68b57ff0fafc@gmail.com> Date: Tue, 8 Nov 2016 17:09:49 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 X-IsSubscribed: yes The -Wformat-length checker relies on the compute_builtin_object_size function to determine the size of the buffer it checks for overflow. The function returns either a size computed by the tree-object-size pass for objects referenced by the __builtin_object_size intrinsic (if it's used in the program) or it tries to compute it for a small subset of expressions otherwise. This subset doesn't include objects allocated by either malloc or alloca, and so for those the function returns "unknown" or (size_t)-1 in the case of -Wformat-length. As a consequence, -Wformat-length is unable to detect overflows involving such objects. The attached patch adds a new function, compute_object_size, that uses the existing algorithms to compute and return the sizes of allocated objects as well, as if they were referenced by __builtin_object_size in the program source, enabling the -Wformat-length checker to detect more buffer overflows. Martin PS The function makes use of the init_function_sizes API that is otherwise unused outside the tree-object-size pass to initialize the internal structures, but then calls fini_object_sizes to release them before returning. That seems wasteful because the size of the same object or one related to it might need to computed again in the context of the same function. I experimented with allocating and releasing the structures only when current_function_decl changes but that led to crashes. I suspect I'm missing something about the management of memory allocated for these structures. Does anyone have any suggestions how to make this work? (Do I perhaps need to allocate them using a special allocator so they don't get garbage collected?) 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. (compute_object_size, internal_object_size): New functions. (compute_builtin_object_size): Call internal_object_size. (pass_object_sizes::execute): Adjust. * tree-object-size.h (compute_object_size): Declare. diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c index 3138ad3..f360711 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -2471,7 +2471,7 @@ get_destination_size (tree dest) object (the function fails without optimization in this type). */ int ost = optimize > 0; unsigned HOST_WIDE_INT size; - if (compute_builtin_object_size (dest, ost, &size)) + if (compute_object_size (dest, ost, &size)) return size; return HOST_WIDE_INT_M1U; 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..59ff90c 100644 --- a/gcc/tree-object-size.c +++ b/gcc/tree-object-size.c @@ -50,6 +50,8 @@ static const unsigned HOST_WIDE_INT unknown[4] = { 0 }; +static bool internal_object_size (tree, int, unsigned HOST_WIDE_INT *); +static void fini_object_sizes (void); 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 +189,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 +493,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 +536,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 +666,38 @@ 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); +} + + +/* Like compute_builtin_object_size but intended to be called + without a corresponding __builtin_object_size in the program. */ + +bool +compute_object_size (tree ptr, int object_size_type, + unsigned HOST_WIDE_INT *psize) +{ + bool init = computed[0] == NULL; + if (init) + init_object_sizes (); + + bool retval = internal_object_size (ptr, object_size_type, psize); + + if (init) + fini_object_sizes (); + + return retval; +} + /* Compute object_sizes for PTR, defined to VALUE, which is not an SSA_NAME. */ static void @@ -1325,7 +1359,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..60256e6 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 bool compute_object_size (tree, int, unsigned HOST_WIDE_INT *); extern bool compute_builtin_object_size (tree, int, unsigned HOST_WIDE_INT *); #endif // GCC_TREE_OBJECT_SIZE_H