From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com [IPv6:2a00:1450:4864:20::62f]) by sourceware.org (Postfix) with ESMTPS id 70F81385841A for ; Tue, 5 Jul 2022 21:58:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 70F81385841A Received: by mail-ej1-x62f.google.com with SMTP id ay16so23946875ejb.6 for ; Tue, 05 Jul 2022 14:58:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=gLoOhFsAeaA/WtQkaS6bANZSih5B3a9RFvnmAAYY3zE=; b=zeYRUZkR0BEHSoeW4FP8JOMmHgHeCIsdv4VB1Usb4sUmrt+RN+XjH3nlLMgBcqkvxH dFef8qf1mw9rg0MR15o5VPGnJwzfweaDoQMGFh2N06Q2joHuklYEdMrI2SeH8ebF09af ECfxOdrB7elFvXuSZ8In739UvVZ7ztXzULhzIVmMhUZignBqa1NqelnRXXutKswSNnE8 ztRxuDs8WbuKgb5EzChck0K3vJz0wmNrPd48WxG+sVKV0grZFbT/el0+NcvqAj9Ya1KD G+SlYVVn/ahpb4sHIQdBNHpPQER7HEbT2RkAfBoQF3wm3EagKRmZ9z0qYi/p592aKV18 xCvg== X-Gm-Message-State: AJIora9NJdJT65GLJ71ho4wQmFhYMUAALMWe6v2y6oDiRBf1aj3QvL4W PtRnoF2sKDvKIm2xxBHh/4VfjQ7cDoqaEchR8MvhdA== X-Google-Smtp-Source: AGRyM1vOA9i6TxAbdJbkraTeIILC3RVn7U6tK736knYTpS4dcKn7w7H0aTIhCcitgnRgGAmZmgVI2YEVUxZf/hyAok8= X-Received: by 2002:a17:907:97c1:b0:726:a646:3b95 with SMTP id js1-20020a17090797c100b00726a6463b95mr34964958ejc.253.1657058314936; Tue, 05 Jul 2022 14:58:34 -0700 (PDT) MIME-Version: 1.0 References: <20220705194909.13675-1-mail@tim-lange.me> <77cd3d305523a14adb7f3cf7f4079b6fb21e429c.camel@redhat.com> In-Reply-To: <77cd3d305523a14adb7f3cf7f4079b6fb21e429c.camel@redhat.com> From: Prathamesh Kulkarni Date: Wed, 6 Jul 2022 03:27:58 +0530 Message-ID: Subject: Re: Floating-point allocation sizes? (was Re: [PATCH] analyzer: Fix handling of non-ints inside allocation size checker [PR106181]) To: David Malcolm Cc: Tim Lange , gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, URI_DOTEDU autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Jul 2022 21:58:39 -0000 On Wed, 6 Jul 2022 at 03:08, David Malcolm via Gcc-patches wrote: > > On Tue, 2022-07-05 at 21:49 +0200, Tim Lange wrote: > > This patch fixes the ICE reported in PR106181 by Arseny Solokha. With > > this patch, the allocation size checker tries to handle floating-point > > operands of allocation size arguments. Constant sizes get implicitly > > converted and symbolic sizes are handled as long as the floating-point > > operand could also be represented as a positive integer. In all other > > cases and on unhandled constants, the checker falls back to not > > emitting a warning. > > Also, I unified the logic on zero byte allocations. > > Hi Tim > > Thanks for the patch. > > We definitely don't want to crash, but my "gut reaction" to the > testsuite examples was that we ought to be warning on them - using > floating point when calculating an allocation size seems like asking > for trouble. > > In particular test_16's: > int32_t *ptr = malloc (n * 3.1); > feels to me like it deserves a warning. I suppose it could be valid if > n is a multiple of 40 (so that the buffer is a multiple of 31 * 4 and > thus a multiple of 4), for small enough n that we don't lose precision, > but that code seems very questionable - the comment says "just assume > that the programmer knows what they are doing", but I think anyone > using -fanalyzer is opting-in to have more fussy checking and would > probably want to be warned about such code. > > I also wondered what happens on NAN, with e.g. > > #include > > void test_nan (void) > { > int *p = malloc (NAN * sizeof (int)); > } > > but we issue -Woverflow on that. > > I'm thinking that perhaps we should have a new warning for floating > point buffer size calculations, though I'm not yet sure exactly how it > should work and how fussy it should be (e.g. complain about floating > point calculations vs complain about *any* floating point used as a > buffer size, etc). Hi David, For the former, I was wondering if it'd be a good idea to complain if alloc_size is floating type, and floor(alloc_size) != alloc_size ? So we warn for: int *p = malloc(4.5); but not for int *p = malloc(4.0); For the latter, I guess -Wconversion should be sufficient ? Thanks, Prathamesh > > Does anyone know of real world code that uses floating point in buffer- > size calculations? (updating Subject accordingly) Is there code out > there that does this? It seems broken to me, but maybe there's a valid > use-case that I can't think of. > > The closest such rule I can think of is CERT-C's > "FLP02-C. Avoid using floating-point numbers when precise computation > is needed": > https://wiki.sei.cmu.edu/confluence/display/c/FLP02-C.+Avoid+using+floating-point+numbers+when+precise+computation+is+needed > > > Dave > > > > > > Regression-tested on x86_64 linux. > > > > 2022-07-05 Tim Lange > > > > gcc/analyzer/ChangeLog: > > > > PR analyzer/106181 > > * region-model.cc (capacity_compatible_with_type): > > Can handle non-integer constants now. > > (region_model::check_region_size): Adapted to the new signature > > of > > capacity_compatible_with_type. > > > > gcc/testsuite/ChangeLog: > > > > PR analyzer/106181 > > * gcc.dg/analyzer/allocation-size-1.c: New tests. > > * gcc.dg/analyzer/allocation-size-2.c: New tests. > > * gcc.dg/analyzer/pr106181.c: New test. > > > > --- > > gcc/analyzer/region-model.cc | 44 ++++++++++++++++--- > > .../gcc.dg/analyzer/allocation-size-1.c | 29 +++++++++++- > > .../gcc.dg/analyzer/allocation-size-2.c | 22 ++++++++++ > > gcc/testsuite/gcc.dg/analyzer/pr106181.c | 7 +++ > > 4 files changed, 95 insertions(+), 7 deletions(-) > > create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr106181.c > > > > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region- > > model.cc > > index 5d939327e01..e097ecb3c07 100644 > > --- a/gcc/analyzer/region-model.cc > > +++ b/gcc/analyzer/region-model.cc > > @@ -2904,13 +2904,45 @@ private: > > > > static bool > > capacity_compatible_with_type (tree cst, tree pointee_size_tree, > > - bool is_struct) > > + bool is_struct, bool floor_real) > > { > > - gcc_assert (TREE_CODE (cst) == INTEGER_CST); > > gcc_assert (TREE_CODE (pointee_size_tree) == INTEGER_CST); > > - > > unsigned HOST_WIDE_INT pointee_size = TREE_INT_CST_LOW > > (pointee_size_tree); > > - unsigned HOST_WIDE_INT alloc_size = TREE_INT_CST_LOW (cst); > > + > > + unsigned HOST_WIDE_INT alloc_size; > > + switch (TREE_CODE (cst)) > > + { > > + default: > > + /* Assume all unhandled operands are compatible. */ > > + return true; > > + case INTEGER_CST: > > + alloc_size = TREE_INT_CST_LOW (cst); > > + break; > > + case REAL_CST: > > + { > > + const REAL_VALUE_TYPE *rv = TREE_REAL_CST_PTR (cst); > > + if (floor_real) > > + { > > + /* If the size is constant real at compile-time, > > + we can model the conversion. */ > > + alloc_size = real_to_integer (rv); > > + } > > + else > > + { > > + /* On expressions where the value of one operator isn't > > + representable as an integer or is negative, we give up > > and > > + just assume that the programmer knows what they are > > doing. */ > > + HOST_WIDE_INT i; > > + if (real_isneg (rv) || !real_isinteger (rv, &i)) > > + return true; > > + alloc_size = i; > > + } > > + } > > + break; > > + } > > + > > + if (alloc_size == 0) > > + return true; > > > > if (is_struct) > > return alloc_size >= pointee_size; > > @@ -2920,7 +2952,7 @@ capacity_compatible_with_type (tree cst, tree > > pointee_size_tree, > > static bool > > capacity_compatible_with_type (tree cst, tree pointee_size_tree) > > { > > - return capacity_compatible_with_type (cst, pointee_size_tree, > > false); > > + return capacity_compatible_with_type (cst, pointee_size_tree, false, > > false); > > } > > > > /* Checks whether SVAL could be a multiple of SIZE_CST. > > @@ -3145,7 +3177,7 @@ region_model::check_region_size (const region > > *lhs_reg, const svalue *rhs_sval, > > = as_a (capacity); > > tree cst_cap = cst_cap_sval->get_constant (); > > if (!capacity_compatible_with_type (cst_cap, pointee_size_tree, > > - is_struct)) > > + is_struct, true)) > > ctxt->warn (new dubious_allocation_size (lhs_reg, rhs_reg, > > cst_cap)); > > } > > diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c > > b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c > > index 4a78a81d054..1a1c8e75c98 100644 > > --- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c > > +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-1.c > > @@ -114,4 +114,31 @@ void test_10 (int32_t n) > > { > > char *ptr = malloc (7 * n); > > free (ptr); > > -} > > \ No newline at end of file > > +} > > + > > +void test_11 () > > +{ > > + int32_t *ptr = malloc (3.0); /* { dg-line malloc11 } */ > > + free (ptr); > > + /* { dg-warning "allocated buffer size is not a multiple of the > > pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc11 } > > */ > > + /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})? > > here; 'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note" > > { target *-*-* } malloc11 } */ > > +} > > + > > +void test_12 () > > +{ > > + int32_t *ptr = malloc (4.0); > > + free (ptr); > > +} > > + > > +void test_13 () > > +{ > > + int32_t *ptr = malloc (4.7); > > + free (ptr); > > +} > > + > > +void test_14 () > > +{ > > + /* Test round towards zero. */ > > + int32_t *ptr = malloc (-0.9); > > + free (ptr); > > +} > > diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c > > b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c > > index d541d5ef8db..babf9ae668d 100644 > > --- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c > > +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-2.c > > @@ -154,3 +154,25 @@ void test_13 (void) > > else > > free (ptr); > > } > > + > > +void test_14 (int32_t n) > > +{ > > + int32_t *ptr = malloc (n * 3.0); /* { dg-line malloc14 } */ > > + free (ptr); > > + /* { dg-warning "allocated buffer size is not a multiple of the > > pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc14 } > > */ > > + /* { dg-message "'int32_t \\*' (\\\{aka '(long )?int \\*'\\\})? > > here; 'sizeof \\(int32_t (\\\{aka (long )?int\\\})?\\)' is '4'" "note" > > { target *-*-* } malloc14 } */ > > +} > > + > > +void test_15 (int32_t n) > > +{ > > + int32_t *ptr = malloc (n * 4.0); > > + free (ptr); > > +} > > + > > +void test_16 (int32_t n) > > +{ > > + /* Should not emit a warning because we can not reason whether the > > result > > + of the floating-point arithmetic actually is a valid size or > > not. */ > > + int32_t *ptr = malloc (n * 3.1); > > + free (ptr); > > +} > > diff --git a/gcc/testsuite/gcc.dg/analyzer/pr106181.c > > b/gcc/testsuite/gcc.dg/analyzer/pr106181.c > > new file mode 100644 > > index 00000000000..6df4e4538c0 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/analyzer/pr106181.c > > @@ -0,0 +1,7 @@ > > +#include > > + > > +int32_t * > > +foo (int32_t x) > > +{ > > + return __builtin_calloc (x * 1.1, 1); > > +} > >