From patchwork Fri Aug 10 15:02:43 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 176508 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]) by ozlabs.org (Postfix) with SMTP id 003532C0091 for ; Sat, 11 Aug 2012 01:03:19 +1000 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1345215800; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject: References:In-Reply-To:Content-Type:Mailing-List:Precedence: List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=ko2lbe7K8v2Gmm5j0Z4PYbmMGQQ=; b=VLwxVfa0oWyLJpT VO2JPvEaXjWlK9Ddeexf+KEghYKEwHHb1r3EkUJEzspf4gEhPmKjjOHmFWk10anU J9JzGZLYQF9kYzXN6+QiLKpwmvZ6QHdl4dZMI4f2hxFW2G1nRtdo632tGzYn1y85 Bd7XDXm5ZAeRpcPIT/LhiBWUKVAU= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:References:In-Reply-To:Content-Type:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=ROJPV6V3PyNjzWx4PYU4bo+ycl1kSRqIYkmris+GBBgWhoncReWdLKA8CbO5kk rvHgNSo+GitC2nBENNSmwb8SLtX0JjMj7zYy2jroXzgkhZDe9NqOtFV9vSvxQD3m KcKZJFZbiLfg4wsKrNzXABAlMdJz+dB/sPSjzOrh3MPSE=; Received: (qmail 30655 invoked by alias); 10 Aug 2012 15:03:13 -0000 Received: (qmail 30643 invoked by uid 22791); 10 Aug 2012 15:03:11 -0000 X-SWARE-Spam-Status: No, hits=-7.7 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, KHOP_THREADED, RCVD_IN_DNSWL_HI, RCVD_IN_HOSTKARMA_W, SPF_HELO_PASS, TW_CL, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 10 Aug 2012 15:02:46 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7AF2khD031036 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 10 Aug 2012 11:02:46 -0400 Received: from dhcp-5-241.str.redhat.com (dhcp-5-241.str.redhat.com [10.32.5.241]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q7AF2ijx010287 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Fri, 10 Aug 2012 11:02:45 -0400 Message-ID: <50252293.6090400@redhat.com> Date: Fri, 10 Aug 2012 17:02:43 +0200 From: Florian Weimer User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0 MIME-Version: 1.0 To: Jason Merrill CC: gcc-patches@gcc.gnu.org Subject: Re: Fix PR c++/19351 (operator new[] overflow) References: <4FD9B4F6.4030200@redhat.com> <4FE9C75A.8030000@redhat.com> <5006C035.8030100@redhat.com> <5006C8A8.1080309@redhat.com> In-Reply-To: <5006C8A8.1080309@redhat.com> X-IsSubscribed: yes 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 On 07/18/2012 04:31 PM, Florian Weimer wrote: > On 07/18/2012 03:55 PM, Jason Merrill wrote: >> On 06/26/2012 10:29 AM, Florian Weimer wrote: >>> + /* Set to (size_t)-1 if the size check fails. */ >>> + if (size_check != NULL_TREE) >>> + *size = fold_build3 (COND_EXPR, sizetype, size_check, >>> + original_size, TYPE_MAX_VALUE (sizetype)); >>> VEC_safe_insert (tree, gc, *args, 0, *size); >>> *args = resolve_args (*args, complain); >>> if (*args == NULL) >>> @@ -4022,7 +4030,11 @@ build_operator_new_call (tree fnname, >>> VEC(tree,gc) **args, >>> if (use_cookie) >>> { >>> /* Update the total size. */ >>> - *size = size_binop (PLUS_EXPR, *size, *cookie_size); >>> + *size = size_binop (PLUS_EXPR, original_size, *cookie_size); >>> + /* Set to (size_t)-1 if the size check fails. */ >>> + gcc_assert (size_check != NULL_TREE); >>> + *size = fold_build3 (COND_EXPR, sizetype, size_check, >>> + *size, TYPE_MAX_VALUE (sizetype)); >> >> Looks like you're evaluating the size_check twice for types that use >> cookies. > > I try to avoid this by using original_size instead of size on the first > assignment under the use_cookie case. > >>> + /* Unconditionally substract the array size. This decreases the >>> + maximum object size and is safe even if we choose not to use >>> + a cookie after all. */ >> >> "cookie size" > > Okay, I will fix that. > >> But since we're going to be deciding whether or not to use a cookie in >> this function anyway, why not do it here? > > The decision to use a cookie is currently split between the two > functions and there are several special cases (Java, ABI compatibility > flags). I did not want to disturb this code too much because we do not > have much test suite coverage in this area. Ping? I'm attaching the current patch. 2012-08-10 Florian Weimer PR c++/19351 * call.c (build_operator_new_call): Add size_check argument and evaluate it. * cp-tree.h (build_operator_new_call): Adjust declaration. * init.c (build_new_1): Compute array size check and apply it. 2012-08-10 Florian Weimer * g++.dg/init/new38.C: New test. * g++.dg/init/new39.C: New test. diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 5345f2b..46a4bcb 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -3941,15 +3941,19 @@ build_new_function_call (tree fn, VEC(tree,gc) **args, bool koenig_p, total number of bytes required by the allocation, and is updated if that is changed here. *COOKIE_SIZE is non-NULL if a cookie should be used. If this function determines that no cookie should be - used, after all, *COOKIE_SIZE is set to NULL_TREE. If FN is - non-NULL, it will be set, upon return, to the allocation function - called. */ + used, after all, *COOKIE_SIZE is set to NULL_TREE. If SIZE_CHECK + is not NULL_TREE, it is evaluated before calculating the final + array size, and if it fails, the array size is replaced with + (size_t)-1 (usually triggering a std::bad_alloc exception). If FN + is non-NULL, it will be set, upon return, to the allocation + function called. */ tree build_operator_new_call (tree fnname, VEC(tree,gc) **args, - tree *size, tree *cookie_size, + tree *size, tree *cookie_size, tree size_check, tree *fn, tsubst_flags_t complain) { + tree original_size = *size; tree fns; struct z_candidate *candidates; struct z_candidate *cand; @@ -3957,6 +3961,10 @@ build_operator_new_call (tree fnname, VEC(tree,gc) **args, if (fn) *fn = NULL_TREE; + /* Set to (size_t)-1 if the size check fails. */ + if (size_check != NULL_TREE) + *size = fold_build3 (COND_EXPR, sizetype, size_check, + original_size, TYPE_MAX_VALUE (sizetype)); VEC_safe_insert (tree, gc, *args, 0, *size); *args = resolve_args (*args, complain); if (*args == NULL) @@ -4020,7 +4028,11 @@ build_operator_new_call (tree fnname, VEC(tree,gc) **args, if (use_cookie) { /* Update the total size. */ - *size = size_binop (PLUS_EXPR, *size, *cookie_size); + *size = size_binop (PLUS_EXPR, original_size, *cookie_size); + /* Set to (size_t)-1 if the size check fails. */ + gcc_assert (size_check != NULL_TREE); + *size = fold_build3 (COND_EXPR, sizetype, size_check, + *size, TYPE_MAX_VALUE (sizetype)); /* Update the argument list to reflect the adjusted size. */ VEC_replace (tree, *args, 0, *size); } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 44f3ac1..f3dccf9 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -4886,7 +4886,7 @@ extern tree build_user_type_conversion (tree, tree, int, extern tree build_new_function_call (tree, VEC(tree,gc) **, bool, tsubst_flags_t); extern tree build_operator_new_call (tree, VEC(tree,gc) **, tree *, - tree *, tree *, + tree *, tree, tree *, tsubst_flags_t); extern tree build_new_method_call (tree, tree, VEC(tree,gc) **, tree, int, tree *, diff --git a/gcc/cp/init.c b/gcc/cp/init.c index a725a0c..09288f8 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -2178,7 +2178,10 @@ build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts, tree pointer_type; tree non_const_pointer_type; tree outer_nelts = NULL_TREE; + /* For arrays, a bounds checks on the NELTS parameter. */ + tree outer_nelts_check = NULL_TREE; bool outer_nelts_from_type = false; + double_int inner_nelts_count = double_int_one; tree alloc_call, alloc_expr; /* The address returned by the call to "operator new". This node is a VAR_DECL and is therefore reusable. */ @@ -2231,7 +2234,22 @@ build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts, { tree inner_nelts = array_type_nelts_top (elt_type); tree inner_nelts_cst = maybe_constant_value (inner_nelts); - if (!TREE_CONSTANT (inner_nelts_cst)) + if (TREE_CONSTANT (inner_nelts_cst) + && TREE_CODE (inner_nelts_cst) == INTEGER_CST) + { + double_int result; + if (mul_double (TREE_INT_CST_LOW (inner_nelts_cst), + TREE_INT_CST_HIGH (inner_nelts_cst), + inner_nelts_count.low, inner_nelts_count.high, + &result.low, &result.high)) + { + if (complain & tf_error) + error ("integer overflow in array size"); + nelts = error_mark_node; + } + inner_nelts_count = result; + } + else { if (complain & tf_error) { @@ -2321,7 +2339,56 @@ build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts, size = size_in_bytes (elt_type); if (array_p) - size = size_binop (MULT_EXPR, size, convert (sizetype, nelts)); + { + /* Maximum available size in bytes. Half of the address space + minus the cookie size. */ + double_int max_size + = double_int_lshift (double_int_one, TYPE_PRECISION (sizetype) - 1, + HOST_BITS_PER_DOUBLE_INT, false); + /* Size of the inner array elements. */ + double_int inner_size; + /* Maximum number of outer elements which can be allocated. */ + double_int max_outer_nelts; + tree max_outer_nelts_tree; + + gcc_assert (TREE_CODE (size) == INTEGER_CST); + cookie_size = targetm.cxx.get_cookie_size (elt_type); + gcc_assert (TREE_CODE (cookie_size) == INTEGER_CST); + gcc_checking_assert (double_int_ucmp + (TREE_INT_CST (cookie_size), max_size) < 0); + /* Unconditionally substract the cookie size. This decreases the + maximum object size and is safe even if we choose not to use + a cookie after all. */ + max_size = double_int_sub (max_size, TREE_INT_CST (cookie_size)); + if (mul_double (TREE_INT_CST_LOW (size), TREE_INT_CST_HIGH (size), + inner_nelts_count.low, inner_nelts_count.high, + &inner_size.low, &inner_size.high) + || double_int_ucmp (inner_size, max_size) > 0) + { + if (complain & tf_error) + error ("size of array is too large"); + return error_mark_node; + } + max_outer_nelts = double_int_udiv (max_size, inner_size, TRUNC_DIV_EXPR); + /* Only keep the top-most seven bits, to simplify encoding the + constant in the instruction stream. */ + { + unsigned shift = HOST_BITS_PER_DOUBLE_INT - 7 + - (max_outer_nelts.high ? clz_hwi (max_outer_nelts.high) + : (HOST_BITS_PER_WIDE_INT + clz_hwi (max_outer_nelts.low))); + max_outer_nelts + = double_int_lshift (double_int_rshift + (max_outer_nelts, shift, + HOST_BITS_PER_DOUBLE_INT, false), + shift, HOST_BITS_PER_DOUBLE_INT, false); + } + max_outer_nelts_tree = double_int_to_tree (sizetype, max_outer_nelts); + + size = size_binop (MULT_EXPR, size, convert (sizetype, nelts)); + outer_nelts_check = fold_build2 (LE_EXPR, boolean_type_node, + outer_nelts, + max_outer_nelts_tree); + } alloc_fn = NULL_TREE; @@ -2384,10 +2451,13 @@ build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts, /* Use a class-specific operator new. */ /* If a cookie is required, add some extra space. */ if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)) - { - cookie_size = targetm.cxx.get_cookie_size (elt_type); - size = size_binop (PLUS_EXPR, size, cookie_size); - } + size = size_binop (PLUS_EXPR, size, cookie_size); + else + cookie_size = NULL_TREE; + /* Perform the overflow check. */ + if (outer_nelts_check != NULL_TREE) + size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check, + size, TYPE_MAX_VALUE (sizetype)); /* Create the argument list. */ VEC_safe_insert (tree, gc, *placement, 0, size); /* Do name-lookup to find the appropriate operator. */ @@ -2418,13 +2488,12 @@ build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts, { /* Use a global operator new. */ /* See if a cookie might be required. */ - if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)) - cookie_size = targetm.cxx.get_cookie_size (elt_type); - else + if (!(array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))) cookie_size = NULL_TREE; alloc_call = build_operator_new_call (fnname, placement, &size, &cookie_size, + outer_nelts_check, &alloc_fn, complain); } } diff --git a/gcc/testsuite/g++.dg/init/new38.C b/gcc/testsuite/g++.dg/init/new38.C new file mode 100644 index 0000000..1672f22 --- /dev/null +++ b/gcc/testsuite/g++.dg/init/new38.C @@ -0,0 +1,54 @@ +// { dg-do compile } + +void +large_array_char(int n) +{ + new char[n] + [1ULL << (sizeof(void *) * 4)] + [1ULL << (sizeof(void *) * 4)]; // { dg-error "size of array" } +} + +template +void +large_array_char_template(int n) +{ + new char[n] + [1ULL << (sizeof(void *) * 4)] + [1ULL << (sizeof(void *) * 4)]; // { dg-error "size of array" } +} + + +template +void +large_array_template1(int n) +{ + new T[n] // { dg-error "size of array is too large" } + [(1ULL << (sizeof(void *) * 4)) / sizeof(T)] + [1ULL << (sizeof(void *) * 4)]; +} + +template +void +large_array_template2(int n) +{ + new T[n] // { dg-error "size of array is too large" } + [(1ULL << (sizeof(void *) * 4)) / sizeof(T)] + [1ULL << (sizeof(void *) * 4)]; +} + +template +void +large_array_template3(int n) +{ + new T[n] // { dg-error "size of array is too large" } + [(1ULL << (sizeof(void *) * 4)) / sizeof(T)] + [1ULL << (sizeof(void *) * 4)]; +} + +void +call_large_array_template(int n) +{ + large_array_template1(n); + large_array_template2(n); + large_array_template3(n); +} diff --git a/gcc/testsuite/g++.dg/init/new39.C b/gcc/testsuite/g++.dg/init/new39.C new file mode 100644 index 0000000..f274ebb --- /dev/null +++ b/gcc/testsuite/g++.dg/init/new39.C @@ -0,0 +1,68 @@ +// Testcase for overflow handling in operator new[]. +// { dg-do run } + +#include +#include + +struct without_new { + char bar[256]; +}; + +struct with_new { + char bar[256]; + void *operator new[] (size_t sz) + { + if (sz != -1) + abort (); + throw std::bad_alloc(); + } +}; + +template +inline void +test (size_t s) +{ + try { + new T[s]; + abort (); + } catch (std::bad_alloc &) { + } +} + +template +void +test_noopt (size_t s) __attribute__((noinline)); + +template +void +test_noopt (size_t s) +{ + __asm__ (""); + test (s); +} + +template +void +all_tests () +{ + test(-1); + test(size_t(-1) / sizeof (T) + 1); + test(size_t(-1) / sizeof (T) + 2); + test_noopt(-1); + test_noopt(size_t(-1) / sizeof (T) + 1); + test_noopt(size_t(-1) / sizeof (T) + 2); +} + +int +main () +{ + try { + ::operator new(size_t(-1)); + abort (); + } catch (std::bad_alloc &) { + } + all_tests (); + all_tests (); + return 0; +} +