From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 89E363858292 for ; Tue, 5 Jul 2022 21:37:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 89E363858292 Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-479-SQXwZnYHMGuJ07VS4fZduQ-1; Tue, 05 Jul 2022 17:37:48 -0400 X-MC-Unique: SQXwZnYHMGuJ07VS4fZduQ-1 Received: by mail-qk1-f198.google.com with SMTP id bp10-20020a05620a458a00b006b265587a17so10904603qkb.2 for ; Tue, 05 Jul 2022 14:37:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=iZZbtHbwZnnIMrLA17g2B1WgrNW9iSlW3n8cCD8vOx0=; b=kg09Lbj8u2OJe22Z62lHVmq3lR4apCbr4PZ0Lw91nK4UdhdAX5EBAL/AX43lHgFzmn i8N7ptvtonR8Qmt+gSY66Uqxa4IU/Ugb1uzCzYd2dv9MGK/+h4NP2leWWxtDkQPvU78I zkdI0gN5R/aqwLbWw8luLI/j8Rq1OKEMUyaROcs46WxxghVvLe4/YB1owtSLQvR6ubz7 oZOt4Oaj//5tDP+7QrL14l+71eUmhOZuif+HHK5cZ0CJG7Lj0cCy3Q1JBsgqwWii3tKJ 3FrMe94eXwEJubQBrZ8gzwEysHvytH07xg4sBNk4N6lqKzKt4Ba1KBR8HqOw4dK96p/7 V8ww== X-Gm-Message-State: AJIora++w0Q9GkRfgk2dp0POQ4tyIJhh7cvA1RLKUWqdY+CwQ/xvVbnw //rHhti8B6CvUzmY5C0Kshv8/nVJ18nzKM4FkEAXyhqSI57hVKSZrjG0NMLO2ihT7bcr3HKUj1N GdFcmTRhFd1YFDsQSrA== X-Received: by 2002:a05:622a:513:b0:31d:3c44:daab with SMTP id l19-20020a05622a051300b0031d3c44daabmr18752926qtx.29.1657057067892; Tue, 05 Jul 2022 14:37:47 -0700 (PDT) X-Google-Smtp-Source: AGRyM1vrfd0IX6smycT7gOfE8r1KOFx5zvyQMKWCwqkFNRPKLnJ7X3vOLLs5GF8ZlpyFgoU/tK0hWw== X-Received: by 2002:a05:622a:513:b0:31d:3c44:daab with SMTP id l19-20020a05622a051300b0031d3c44daabmr18752904qtx.29.1657057067515; Tue, 05 Jul 2022 14:37:47 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id x4-20020ac87a84000000b00307cac53129sm22698408qtr.42.2022.07.05.14.37.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Jul 2022 14:37:46 -0700 (PDT) Message-ID: <77cd3d305523a14adb7f3cf7f4079b6fb21e429c.camel@redhat.com> Subject: Floating-point allocation sizes? (was Re: [PATCH] analyzer: Fix handling of non-ints inside allocation size checker [PR106181]) From: David Malcolm To: Tim Lange , gcc-patches@gcc.gnu.org Date: Tue, 05 Jul 2022 17:37:46 -0400 In-Reply-To: <20220705194909.13675-1-mail@tim-lange.me> References: <20220705194909.13675-1-mail@tim-lange.me> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, 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:37:52 -0000 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). 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); > +}