From patchwork Fri Oct 9 02:55:08 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 528080 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id CD18A140DA6 for ; Fri, 9 Oct 2015 13:55:28 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=XxXPyboM; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:content-type; q= dns; s=default; b=lTiKIVAnCHPCTiMKPwkNe5ugr/fR0fC98YNpPkUvD2Pkzm Ag7K3jexI9OOUzwt4wlREWcDbguXp9R7pxb+aNS1zVQBTlqS/+AyxvyRGH99RJmk gJYfGpKspg8iWChnk8RldswRJsOFEgZ024ccui+NU2YKuz9dI0WtKj1xZ9Jt4= 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 :message-id:date:from:mime-version:to:subject:content-type; s= default; bh=ZQjODYj23kYkmugR8Njj0nlL3UE=; b=XxXPyboMKkLV4bXGuBNh kxZotV3sBaqX94+XQTR26WB8OZOcLFsigpIzTOnn/xa71H9KvGEM2JlDiZ2627Ls jDH8zXQxjGlMHi0BSXI+o67AJ6gFizfGjhm5+PcvbzNhIyrzxoIGEMYiwSYF+Hae nrMCbGdVVIyZ3ND0+vaRM8s= Received: (qmail 35771 invoked by alias); 9 Oct 2015 02:55:18 -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 35750 invoked by uid 89); 9 Oct 2015 02:55:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qk0-f172.google.com Received: from mail-qk0-f172.google.com (HELO mail-qk0-f172.google.com) (209.85.220.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 09 Oct 2015 02:55:13 +0000 Received: by qkht68 with SMTP id t68so27508477qkh.3 for ; Thu, 08 Oct 2015 19:55:11 -0700 (PDT) X-Received: by 10.55.200.204 with SMTP id t73mr7753368qkl.87.1444359311192; Thu, 08 Oct 2015 19:55:11 -0700 (PDT) Received: from [192.168.0.26] (97-124-165-221.hlrn.qwest.net. [97.124.165.221]) by smtp.gmail.com with ESMTPSA id f107sm20082695qge.23.2015.10.08.19.55.09 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 08 Oct 2015 19:55:10 -0700 (PDT) Message-ID: <56172C8C.2070202@gmail.com> Date: Thu, 08 Oct 2015 20:55:08 -0600 From: Martin Sebor User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Gcc Patch List Subject: [PATCH] c/67882 - improve -Warray-bounds for invalid offsetof X-IsSubscribed: yes Gcc attempts to diagnose invalid offsetof expressions whose member designator is an array element with an out-of-bounds index. The logic in the function that does this detection is incomplete, leading to false negatives. Since the result of the expression in these cases can be surprising, this patch tightens up the logic to diagnose more such cases. Tested by boostrapping and running c/c++ tests on x86_64 with no regressions. Martin diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 4b922bf..fc7c991d 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -10536,12 +10536,31 @@ c_common_to_target_charset (HOST_WIDE_INT c) /* Fold an offsetof-like expression. EXPR is a nested sequence of component references with an INDIRECT_REF of a constant at the bottom; much like the - traditional rendering of offsetof as a macro. Return the folded result. */ + traditional rendering of offsetof as a macro. Return the folded result. + PCTX, which is initially null, is set by the first recursive call of + the function to refer to a local object describing the potentially out- + of-bounds index of the array member whose offset is being computed, and + to indicate whether all indices to the same array object have the highest + valid value. The function issues a warning for out-of-bounds array indices + that either refer to elements past the one just past end of the array object + or that exceed any of the major bounds. */ + +struct offsetof_ctx_t +{ + tree inx; /* The invalid array index or NULL_TREE. */ + int maxinx; /* All indices to the array have the highest valid value. */ +}; tree -fold_offsetof_1 (tree expr) +fold_offsetof_1 (tree expr, offsetof_ctx_t *pctx /* = 0 */) { tree base, off, t; + offsetof_ctx_t ctx = { NULL_TREE, -1 }; + + /* Set the context pointer to point to the local context object + to use by subsequent recursive calls. */ + if (!pctx) + pctx = &ctx; switch (TREE_CODE (expr)) { @@ -10567,10 +10586,19 @@ fold_offsetof_1 (tree expr) return TREE_OPERAND (expr, 0); case COMPONENT_REF: - base = fold_offsetof_1 (TREE_OPERAND (expr, 0)); + base = fold_offsetof_1 (TREE_OPERAND (expr, 0), pctx); if (base == error_mark_node) return base; + if (ctx.inx != NULL_TREE) { + warning (OPT_Warray_bounds, + "index %E denotes an offset " + "greater than size of %qT", + ctx.inx, TREE_TYPE (TREE_OPERAND (expr, 0))); + /* Reset to avoid diagnosing the same expression multiple times. */ + pctx->inx = NULL_TREE; + } + t = TREE_OPERAND (expr, 1); if (DECL_C_BIT_FIELD (t)) { @@ -10581,10 +10609,11 @@ fold_offsetof_1 (tree expr) off = size_binop_loc (input_location, PLUS_EXPR, DECL_FIELD_OFFSET (t), size_int (tree_to_uhwi (DECL_FIELD_BIT_OFFSET (t)) / BITS_PER_UNIT)); + pctx->maxinx = -1; break; case ARRAY_REF: - base = fold_offsetof_1 (TREE_OPERAND (expr, 0)); + base = fold_offsetof_1 (TREE_OPERAND (expr, 0), pctx); if (base == error_mark_node) return base; @@ -10601,8 +10630,10 @@ fold_offsetof_1 (tree expr) { upbound = size_binop (PLUS_EXPR, upbound, build_int_cst (TREE_TYPE (upbound), 1)); - if (tree_int_cst_lt (upbound, t)) - { + + if (tree_int_cst_lt (upbound, t)) { + pctx->inx = t; + tree v; for (v = TREE_OPERAND (expr, 0); @@ -10622,25 +10653,61 @@ fold_offsetof_1 (tree expr) /* Don't warn if the array might be considered a poor man's flexible array member with a very permissive definition thereof. */ - if (TREE_CODE (v) == ARRAY_REF - || TREE_CODE (v) == COMPONENT_REF) + if (TREE_CODE (v) != ARRAY_REF + && TREE_CODE (v) != COMPONENT_REF) + pctx = NULL; + } + else if (pctx->inx == NULL_TREE && tree_int_cst_equal (upbound, t)) + { + /* Index is considered valid when it's either less than + the upper bound or equal to it and refers to the lowest + rank. Since in the latter case it may not at this point + in the recursive call to the function be known whether + the element at the index is used to do more than to + compute its offset (e.g., it can be used to designate + a member of the type to which the just past-the-end + element refers), point the INX variable at the index + and leave it up to the caller to decide whether or not + to diagnose it. Special handling is required for minor + index values referring to the element just past the end + of the array object. */ + pctx->inx = t; + tree_code lastcode = TREE_CODE (TREE_OPERAND (expr, 0)); + if ((lastcode != ARRAY_REF && pctx != &ctx) + || (pctx == &ctx && pctx->maxinx)) + pctx = NULL; + } + else + { + HOST_WIDE_INT ubi = tree_to_shwi (upbound); + HOST_WIDE_INT inx = tree_to_shwi (t); + + if (pctx->maxinx) + pctx->maxinx = inx + 1 == ubi; + } + } + } + + if (pctx && pctx->inx) + { warning (OPT_Warray_bounds, "index %E denotes an offset " "greater than size of %qT", - t, TREE_TYPE (TREE_OPERAND (expr, 0))); - } - } + pctx->inx, TREE_TYPE (TREE_OPERAND (expr, 0))); + + pctx->inx = NULL_TREE; } t = convert (sizetype, t); off = size_binop (MULT_EXPR, TYPE_SIZE_UNIT (TREE_TYPE (expr)), t); + break; case COMPOUND_EXPR: /* Handle static members of volatile structs. */ t = TREE_OPERAND (expr, 1); gcc_assert (VAR_P (t)); - return fold_offsetof_1 (t); + return fold_offsetof_1 (t, pctx); default: gcc_unreachable (); diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 74d1bc1..72e2f95 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1015,7 +1015,8 @@ extern bool c_dump_tree (void *, tree); extern void verify_sequence_points (tree); -extern tree fold_offsetof_1 (tree); +struct offsetof_ctx_t; +extern tree fold_offsetof_1 (tree, offsetof_ctx_t* = NULL); extern tree fold_offsetof (tree); /* Places where an lvalue, or modifiable lvalue, may be required. diff --git a/gcc/testsuite/c-c++-common/builtin-offsetof-2.c b/gcc/testsuite/c-c++-common/builtin-offsetof-2.c new file mode 100644 index 0000000..09979fd --- /dev/null +++ b/gcc/testsuite/c-c++-common/builtin-offsetof-2.c @@ -0,0 +1,157 @@ +// { dg-options "-Warray-bounds" } +// { dg-do compile } + +// Test case exercising pr c/67882 - surprising offsetof result +// on an invalid array member without diagnostic. + +typedef struct A { + char a1_1[1][1]; + char a[]; +} A; + +typedef struct A2 { + char a1_1[1][1]; + char c; +} A2; + +typedef struct B { + A2 a2_3[2][3]; + char a1_1[3][5]; + char a[]; +} B; + +// Structures with members that contain flexible array members are +// an extension accepted by GCC. +typedef struct C { + A a5_7 [5][7]; + int a; +} C; + +// Structs with a "fake" flexible array member (a GCC extension). +typedef struct FA0 { + int i; + char a0 [0]; +} FA0; + +typedef struct FA1 { + int i; + char a1 [1]; +} FA1; + +typedef struct FA3 { + int i; + char a3 [3]; +} FA3; + +// A "fake" multidimensional flexible array member deserves special +// treatment since the offsetof exression yields the same result for +// references to apparently distinct members (e.g., a5_7[0][7] is +// at the same offset as a5_7[1][0] which may be surprising). +typedef struct FA5_7 { + int i; + char a5_7 [5][7]; +} FA5_7; + +static void test (void) +{ + // Verify that offsetof references to array elements past the end of + // the array member are diagnosed. As an extension, permit references + // to the element just past-the-end of the array. + + int a[] = { + __builtin_offsetof (A, a), // valid + __builtin_offsetof (A, a [0]), // valid + __builtin_offsetof (A, a [1]), // valid + __builtin_offsetof (A, a [99]), // valid + + __builtin_offsetof (A, a1_1 [0][0]), // valid + + // The following expression is silently accepted as an extension + // because it simply forms the equivalent of a just-past-the-end + // address. + __builtin_offsetof (A, a1_1 [0][1]), // extension + + __builtin_offsetof (A, a1_1 [1][0]), // { dg-warning "index" } + __builtin_offsetof (A, a1_1 [1][1]), // { dg-warning "index" } + + __builtin_offsetof (B, a2_3 [0][0].c), // valid + __builtin_offsetof (B, a2_3 [0][0].a1_1 [0][0]), // valid + + // The following expressions are silently accepted as an extension + // because they simply form the equivalent of a just-past-the-end + // address. + __builtin_offsetof (B, a2_3 [1][3]), // extension + __builtin_offsetof (B, a2_3 [0][0].a1_1 [0][1]), // extension + + __builtin_offsetof (B, a2_3 [0][0].a1_1 [1][0]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [0][0].a1_1 [1][1]), // { dg-warning "index" } + + __builtin_offsetof (B, a2_3 [1][2].a1_1 [0][0]), // valid + + // Forming an offset to the just-past-end element is accepted. + __builtin_offsetof (B, a2_3 [1][2].a1_1 [0][1]), // extension + __builtin_offsetof (B, a2_3 [1][2].a1_1 [1][0]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [1][2].a1_1 [1][1]), // { dg-warning "index" } + + // Forming an offset to the just-past-end element is accepted. + __builtin_offsetof (B, a2_3 [1][3]), // exension + // ...but these are diagnosed because they dereference a just-path-the-end + // element. + __builtin_offsetof (B, a2_3 [1][3].a1_1 [0][0]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [1][3].a1_1 [0][0]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [1][3].a1_1 [0][1]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [1][3].a1_1 [1][0]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [1][3].a1_1 [1][1]), // { dg-warning "index" } + + // Analogous to the case above, these are both diagnosed because they + // dereference just-path-the-end elements of the a2_3 array. + __builtin_offsetof (B, a2_3 [1][3].c), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [1][3].c), // { dg-warning "index" } + + // The following are all invalid because of the reference to a2_3[2]. + __builtin_offsetof (B, a2_3 [2][0].a1_1 [0][0]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [2][0].a1_1 [0][1]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [2][0].a1_1 [1][0]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [2][0].a1_1 [1][1]), // { dg-warning "index" } + __builtin_offsetof (B, a2_3 [2][0].c), // { dg-warning "index" } + + __builtin_offsetof (C, a5_7 [4][6]), + __builtin_offsetof (C, a5_7 [4][6].a), + __builtin_offsetof (C, a5_7 [4][6].a [0]), + __builtin_offsetof (C, a5_7 [4][6].a [99]), + + __builtin_offsetof (C, a5_7 [4][7]), // extension + // Diagnose the following even though the object whose offset is + // computed is a flexible array member. + __builtin_offsetof (C, a5_7 [4][7].a), // { dg-warning "index" } + __builtin_offsetof (C, a5_7 [4][7].a [0]), // { dg-warning "index" } + __builtin_offsetof (C, a5_7 [4][7].a [99]), // { dg-warning "index" } + + // Verify that no diagnostic is issued for offsetof expressions + // involving structs where the array has a rank of 1 and is the last + // member (e.g., those are treated as flexible array members). + __builtin_offsetof (FA0, a0 [0]), + __builtin_offsetof (FA0, a0 [1]), + __builtin_offsetof (FA0, a0 [99]), + + __builtin_offsetof (FA1, a1 [0]), + __builtin_offsetof (FA1, a1 [1]), + __builtin_offsetof (FA1, a1 [99]), + + __builtin_offsetof (FA3, a3 [0]), + __builtin_offsetof (FA3, a3 [3]), + __builtin_offsetof (FA3, a3 [99]), + + __builtin_offsetof (FA5_7, a5_7 [0][0]), + + // Unlike one-dimensional arrays, verify that out-of-bounds references + // to arrays with rank of 2 and greater are diagnosed. + __builtin_offsetof (FA5_7, a5_7 [0][7]), // { dg-warning "index" } + __builtin_offsetof (FA5_7, a5_7 [1][7]), // { dg-warning "index" } + __builtin_offsetof (FA5_7, a5_7 [5][0]), // { dg-warning "index" } + __builtin_offsetof (FA5_7, a5_7 [5][7]), // { dg-warning "index" } + __builtin_offsetof (FA5_7, a5_7 [99][99]) // { dg-warning "index" } + }; + + (void)&a; +}