From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x536.google.com (mail-pg1-x536.google.com [IPv6:2607:f8b0:4864:20::536]) by sourceware.org (Postfix) with ESMTPS id 592FA3858438 for ; Fri, 9 Jun 2023 00:20:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 592FA3858438 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pg1-x536.google.com with SMTP id 41be03b00d2f7-53063897412so286683a12.0 for ; Thu, 08 Jun 2023 17:20:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686270008; x=1688862008; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=XPoWuROLxkfwsW4pCtnWvP4q/W3/SEQKF7AFjV5+RGk=; b=bAqPrrcdfv5Fs1c0H6cRhi/3USk82NUPm5KnB2esTtfHUOv0aptBZagn0j3Z3GnRQS SzafqW6HbYk1D7RDXFa5BKofui5Y3WLN8cA9AqMnewmIRDJibUWEl35he0k8Wfq5WNdG tKEVkbqjyo4ds2AHcJa4QIxDJBUIu/4sQfbUifLwqLzt6ESlerF0M+M/qByDBPfVYtw4 2TksvQ7Og5b53teQcebq8uwreLKCRi5E94+TFfL0XHw6Hikj0oRfUxlyS73SUK1KgTgu vNNQZulr9tTT7YnSRzfTj0IHxtZMFQbar3aNJ7eE1rHtgIJjUwjC3Uxiy6tw+DNXRDtQ 48uA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686270008; x=1688862008; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=XPoWuROLxkfwsW4pCtnWvP4q/W3/SEQKF7AFjV5+RGk=; b=IWRYhBW/xHAFhLjkRU+Tdvdkm+0gL7fZFIVeVOnpDDHg6Nq34xUhc/jSSkSL6obDRy 6zPrkvCj3Pc6wkzJisuY1P8Jrn8f7bk0ddOJFEEtlmhOZx61YvvmFY+RqtQnzw6VQQMj r84aGkEXYPk8bS/6ysceONAhRvU6JPzGirupeKQE83tIyoHj4Kdrn3X466kyf7pkXRGB dKvMntuAAS4/Vji0W9+8vuDHrQhryUFZiEcFEbJlsNr1q8ki/g9tMrcU0eWVTwnLHPfj yRJbn7yf/fTP15fUMWE6C1HsvmOyw91LuxFzsWXVzeW5uemZWdUMzeiRbLKBRv3ri3pq FX0w== X-Gm-Message-State: AC+VfDw2seHpp3IKYWR4Khhwc+tLr0NxuTQfIxgMi4OMNBzqGivEbiQz jKHt339G3gZaYbMtU3c/MUjYhqEVWc82oD3zFA== X-Google-Smtp-Source: ACHHUZ7ENOZbpX1CHfWXLdSIi2VoQfuLAIW5WLXailEZKOTjjSX+lVp4h+2osK5/MrfUfG9dJPVknMnZ6BKXmmFWNr0= X-Received: by 2002:a17:90b:1046:b0:253:9754:bed1 with SMTP id gq6-20020a17090b104600b002539754bed1mr67216pjb.6.1686270007693; Thu, 08 Jun 2023 17:20:07 -0700 (PDT) MIME-Version: 1.0 References: <20230606114858.447221-1-vultkayn@gcc.gnu.org> <225A7AAD-E31D-4BF1-AC1F-CE160989FE60@linaro.org> In-Reply-To: From: Benjamin Priour Date: Fri, 9 Jun 2023 02:19:55 +0200 Message-ID: Subject: Fwd: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439] To: David Malcolm Cc: gcc-patches@gcc.gnu.org Content-Type: multipart/alternative; boundary="000000000000cdd64c05fda754b3" X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --000000000000cdd64c05fda754b3 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi David, So first real committed patch actually was a misstep. So I'm currently fixing that. The issue is that the original idea, to return a boolean and create a unknown_svalue on OOB access to prevent further "use-of-uninitialized-value" caused a loss of information on the location of the buffer. So now, when a buffer is on the stack, we lose that information by returning an unknown_svalue from get_store_value. Therefore further checks from 'sm_malloc' won't be able to detect erroneous operations expecting heap-allocated buffer, e.g. a delete. It does not trouble successive out_of_bounds checks, since the checks are done on the boundaries of the initial buffer, that The issue is from sm_state_map::get_state, since an unknown_svalue cannot hold any state, then in the checker is misled. I thought to artificially add a state, but since the unknown_svalue are singleton per type, it is not right. Therefore I'm considering something, to make it so can_have_initial_svalue_p holds true for OOB heap access as it is for the stack, instead of creating an unknown_svalue. I'll do **PROPER** testing tomorrow, now that I have the compile farm, to check if doing so won't introduce any further issue. This way we would keep all the relevant information about the region, without making it poisoned, and OOB checks are done with the initial byte size of the buffer, so this should not be disturbed. I briefly tried the above as a proof of concept. Doing so fixed PR100244.C mentioned by Maxim, while still passing my new test cases for PR109439. I will regtest this configuration tomorrow morning on the farm, I am getting sleepy, except if you can already see problems this would cause. Sorry again I have somewhat managed to fail my first commit, and pushed it. Thanks, Benjamin. ---------- Forwarded message --------- From: Benjamin Priour Date: Thu, Jun 8, 2023 at 8:18=E2=80=AFPM Subject: Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439] To: Maxim Kuvyrkov Cc: , Benjamin Priour , < dmalcolm@redhat.com> Hi, Yes of course, I tested many days ago since regtesting takes several days on my box, I should have retested ! But I got an account for the compile farm today, so I'm on it immediately, I also see a divergence in the warnings on my box. Thanks for the report ! Sincerely sorry, Benjamin. On Thu, Jun 8, 2023 at 7:53=E2=80=AFPM Maxim Kuvyrkov wrote: > > On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches < > gcc-patches@gcc.gnu.org> wrote: > > > > From: Benjamin Priour > > > > This patch enchances -Wanalyzer-out-of-bounds that is no longer paired > > with a -Wanalyzer-use-of-uninitialized-value on out-of-bounds-read. > > > > This also fixes PR analyzer/109437. > > Before there could always be at most one OOB-read warning per frame > because > > -Wanalyzer-use-of-uninitialized-value always terminates the analysis > > path. > > > > PR analyzer/109439 > > > > gcc/analyzer/ChangeLog: > > > > * bounds-checking.cc (region_model::check_symbolic_bounds): > > Returns whether the BASE_REG region access was OOB. > > (region_model::check_region_bounds): Likewise. > > * region-model.cc (region_model::get_store_value): Creates an > > unknown svalue on OOB-read access to REG. > > (region_model::check_region_access): Returns whether an > > unknown svalue needs be created. > > (region_model::check_region_for_read): Passes > > check_region_access return value. > > * region-model.h: Update prior function definitions. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/analyzer/out-of-bounds-2.c: Cleaned test for > > uninitialized-value warning. > > * gcc.dg/analyzer/out-of-bounds-5.c: Likewise. > > * gcc.dg/analyzer/pr101962.c: Likewise. > > * gcc.dg/analyzer/realloc-5.c: Likewise. > > * gcc.dg/analyzer/pr109439.c: New test. > > --- > > Hi Benjamin, > > This patch makes two tests fail on arm-linux-gnueabihf. Probably, they > need to be updated as well. Would you please investigate? Let me know if > it doesn't easily reproduce for you, and I'll help. > > =3D=3D=3D g++ tests =3D=3D=3D > > Running g++:g++.dg/analyzer/analyzer.exp ... > FAIL: g++.dg/analyzer/pr100244.C -std=3Dc++14 (test for warnings, line 17) > FAIL: g++.dg/analyzer/pr100244.C -std=3Dc++17 (test for warnings, line 17) > FAIL: g++.dg/analyzer/pr100244.C -std=3Dc++20 (test for warnings, line 17) > > =3D=3D=3D gcc tests =3D=3D=3D > > Running gcc:gcc.dg/analyzer/analyzer.exp ... > FAIL: gcc.dg/analyzer/pr101962.c (test for warnings, line 19) > > Thanks, > > -- > Maxim Kuvyrkov > https://www.linaro.org > > > > > gcc/analyzer/bounds-checking.cc | 30 +++++++++++++------ > > gcc/analyzer/region-model.cc | 22 +++++++++----- > > gcc/analyzer/region-model.h | 8 ++--- > > .../gcc.dg/analyzer/out-of-bounds-2.c | 1 - > > .../gcc.dg/analyzer/out-of-bounds-5.c | 2 -- > > gcc/testsuite/gcc.dg/analyzer/pr101962.c | 1 - > > gcc/testsuite/gcc.dg/analyzer/pr109439.c | 12 ++++++++ > > gcc/testsuite/gcc.dg/analyzer/realloc-5.c | 1 - > > 8 files changed, 51 insertions(+), 26 deletions(-) > > create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr109439.c > > > > diff --git a/gcc/analyzer/bounds-checking.cc > b/gcc/analyzer/bounds-checking.cc > > index 3bf542a8eba..479b8e4b88d 100644 > > --- a/gcc/analyzer/bounds-checking.cc > > +++ b/gcc/analyzer/bounds-checking.cc > > @@ -767,9 +767,11 @@ public: > > } > > }; > > > > -/* Check whether an access is past the end of the BASE_REG. */ > > +/* Check whether an access is past the end of the BASE_REG. > > + Return TRUE if the access was valid, FALSE otherwise. > > +*/ > > > > -void > > +bool > > region_model::check_symbolic_bounds (const region *base_reg, > > const svalue *sym_byte_offset, > > const svalue *num_bytes_sval, > > @@ -800,6 +802,7 @@ region_model::check_symbolic_bounds (const region > *base_reg, > > offset_tree, > > num_bytes_tree, > > capacity_tree)); > > + return false; > > break; > > case DIR_WRITE: > > ctxt->warn (make_unique (base_reg, > > @@ -807,9 +810,11 @@ region_model::check_symbolic_bounds (const region > *base_reg, > > offset_tree, > > num_bytes_tree, > > capacity_tree)); > > + return false; > > break; > > } > > } > > + return true; > > } > > > > static tree > > @@ -822,9 +827,11 @@ maybe_get_integer_cst_tree (const svalue *sval) > > return NULL_TREE; > > } > > > > -/* May complain when the access on REG is out-of-bounds. */ > > +/* May complain when the access on REG is out-of-bounds. > > + Return TRUE if the access was valid, FALSE otherwise. > > +*/ > > > > -void > > +bool > > region_model::check_region_bounds (const region *reg, > > enum access_direction dir, > > region_model_context *ctxt) const > > @@ -839,14 +846,14 @@ region_model::check_region_bounds (const region > *reg, > > (e.g. because the analyzer did not see previous offsets on the > latter, > > it might think that a negative access is before the buffer). */ > > if (base_reg->symbolic_p ()) > > - return; > > + return true; > > > > /* Find out how many bytes were accessed. */ > > const svalue *num_bytes_sval =3D reg->get_byte_size_sval (m_mgr); > > tree num_bytes_tree =3D maybe_get_integer_cst_tree (num_bytes_sval); > > /* Bail out if 0 bytes are accessed. */ > > if (num_bytes_tree && zerop (num_bytes_tree)) > > - return; > > + return true; > > > > /* Get the capacity of the buffer. */ > > const svalue *capacity =3D get_capacity (base_reg); > > @@ -877,13 +884,13 @@ region_model::check_region_bounds (const region > *reg, > > } > > else > > byte_offset_sval =3D reg_offset.get_symbolic_byte_offset (); > > - check_symbolic_bounds (base_reg, byte_offset_sval, num_bytes_sva= l, > > + return check_symbolic_bounds (base_reg, byte_offset_sval, > num_bytes_sval, > > capacity, dir, ctxt); > > - return; > > } > > > > /* Otherwise continue to check with concrete values. */ > > byte_range out (0, 0); > > + bool oob_safe =3D true; > > /* NUM_BYTES_TREE should always be interpreted as unsigned. */ > > byte_offset_t num_bytes_unsigned =3D wi::to_offset (num_bytes_tree); > > byte_range read_bytes (offset, num_bytes_unsigned); > > @@ -899,10 +906,12 @@ region_model::check_region_bounds (const region > *reg, > > case DIR_READ: > > ctxt->warn (make_unique (reg, diag_arg, > > out)); > > + oob_safe =3D false; > > break; > > case DIR_WRITE: > > ctxt->warn (make_unique (reg, diag_arg, > > out)); > > + oob_safe =3D false; > > break; > > } > > } > > @@ -911,7 +920,7 @@ region_model::check_region_bounds (const region *re= g, > > do a symbolic check here because the inequality check does not > reason > > whether constants are greater than symbolic values. */ > > if (!cst_capacity_tree) > > - return; > > + return oob_safe; > > > > byte_range buffer (0, wi::to_offset (cst_capacity_tree)); > > /* If READ_BYTES exceeds BUFFER, we do have an overflow. */ > > @@ -929,13 +938,16 @@ region_model::check_region_bounds (const region > *reg, > > case DIR_READ: > > ctxt->warn (make_unique (reg, diag_arg, > > out, byte_bound)); > > + oob_safe =3D false; > > break; > > case DIR_WRITE: > > ctxt->warn (make_unique (reg, diag_arg, > > out, byte_bound)); > > + oob_safe =3D false; > > break; > > } > > } > > + return oob_safe; > > } > > > > } // namespace ana > > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc > > index 3bb3df2f063..fb96cd54940 100644 > > --- a/gcc/analyzer/region-model.cc > > +++ b/gcc/analyzer/region-model.cc > > @@ -2396,7 +2396,8 @@ region_model::get_store_value (const region *reg, > > if (reg->empty_p ()) > > return m_mgr->get_or_create_unknown_svalue (reg->get_type ()); > > > > - check_region_for_read (reg, ctxt); > > + if (check_region_for_read (reg, ctxt)) > > + return m_mgr->get_or_create_unknown_svalue(reg->get_type()); > > > > /* Special-case: handle var_decls in the constant pool. */ > > if (const decl_region *decl_reg =3D reg->dyn_cast_decl_region ()) > > @@ -2802,19 +2803,22 @@ region_model::get_string_size (const region > *reg) const > > } > > > > /* If CTXT is non-NULL, use it to warn about any problems accessing REG, > > - using DIR to determine if this access is a read or write. */ > > + using DIR to determine if this access is a read or write. > > + Return TRUE if an UNKNOWN_SVALUE needs be created. */ > > > > -void > > +bool > > region_model::check_region_access (const region *reg, > > enum access_direction dir, > > region_model_context *ctxt) const > > { > > /* Fail gracefully if CTXT is NULL. */ > > if (!ctxt) > > - return; > > + return false; > > > > + bool need_unknown_sval =3D false; > > check_region_for_taint (reg, dir, ctxt); > > - check_region_bounds (reg, dir, ctxt); > > + if (!check_region_bounds (reg, dir, ctxt)) > > + need_unknown_sval =3D true; > > > > switch (dir) > > { > > @@ -2827,6 +2831,7 @@ region_model::check_region_access (const region > *reg, > > check_for_writable_region (reg, ctxt); > > break; > > } > > + return need_unknown_sval; > > } > > > > /* If CTXT is non-NULL, use it to warn about any problems writing to > REG. */ > > @@ -2838,13 +2843,14 @@ region_model::check_region_for_write (const > region *dest_reg, > > check_region_access (dest_reg, DIR_WRITE, ctxt); > > } > > > > -/* If CTXT is non-NULL, use it to warn about any problems reading from > REG. */ > > +/* If CTXT is non-NULL, use it to warn about any problems reading from > REG. > > + Returns TRUE if an unknown svalue needs be created. */ > > > > -void > > +bool > > region_model::check_region_for_read (const region *src_reg, > > region_model_context *ctxt) const > > { > > - check_region_access (src_reg, DIR_READ, ctxt); > > + return check_region_access (src_reg, DIR_READ, ctxt); > > } > > > > /* Concrete subclass for casts of pointers that lead to trailing bytes. > */ > > diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h > > index fe3db0b0c98..12f84b20463 100644 > > --- a/gcc/analyzer/region-model.h > > +++ b/gcc/analyzer/region-model.h > > @@ -553,22 +553,22 @@ private: > > > > void check_for_writable_region (const region* dest_reg, > > region_model_context *ctxt) const; > > - void check_region_access (const region *reg, > > + bool check_region_access (const region *reg, > > enum access_direction dir, > > region_model_context *ctxt) const; > > - void check_region_for_read (const region *src_reg, > > + bool check_region_for_read (const region *src_reg, > > region_model_context *ctxt) const; > > void check_region_size (const region *lhs_reg, const svalue *rhs_sval, > > region_model_context *ctxt) const; > > > > /* Implemented in bounds-checking.cc */ > > - void check_symbolic_bounds (const region *base_reg, > > + bool check_symbolic_bounds (const region *base_reg, > > const svalue *sym_byte_offset, > > const svalue *num_bytes_sval, > > const svalue *capacity, > > enum access_direction dir, > > region_model_context *ctxt) const; > > - void check_region_bounds (const region *reg, enum access_direction > dir, > > + bool check_region_bounds (const region *reg, enum access_direction > dir, > > region_model_context *ctxt) const; > > > > void check_call_args (const call_details &cd) const; > > diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c > b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c > > index 1330090f348..336f624441c 100644 > > --- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c > > +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-2.c > > @@ -82,5 +82,4 @@ void test5 (void) > > /* { dg-warning "heap-based buffer over-read" "bounds warning" { > target *-*-* } test5 } */ > > /* { dg-message "read of 4 bytes from after the end of the region" > "num bad bytes note" { target *-*-* } test5 } */ > > > > - /* { dg-warning "use of uninitialized value" "uninit warning" { > target *-*-* } test5 } */ > > } > > diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c > b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c > > index 2a61d8ca236..568f9cad199 100644 > > --- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c > > +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-5.c > > @@ -68,7 +68,6 @@ void test8 (size_t size, size_t offset) > > char dst[size]; > > memcpy (dst, src, size + offset); /* { dg-line test8 } */ > > /* { dg-warning "over-read" "warning" { target *-*-* } test8 } */ > > - /* { dg-warning "use of uninitialized value" "warning" { target *-*-* > } test8 } */ > > /* { dg-warning "overflow" "warning" { target *-*-* } test8 } */ > > } > > > > @@ -78,7 +77,6 @@ void test9 (size_t size, size_t offset) > > int32_t dst[size]; > > memcpy (dst, src, 4 * size + 1); /* { dg-line test9 } */ > > /* { dg-warning "over-read" "warning" { target *-*-* } test9 } */ > > - /* { dg-warning "use of uninitialized value" "warning" { target *-*-* > } test9 } */ > > /* { dg-warning "overflow" "warning" { target *-*-* } test9 } */ > > } > > > > diff --git a/gcc/testsuite/gcc.dg/analyzer/pr101962.c > b/gcc/testsuite/gcc.dg/analyzer/pr101962.c > > index 08c0aba5cbf..b878aad9cf1 100644 > > --- a/gcc/testsuite/gcc.dg/analyzer/pr101962.c > > +++ b/gcc/testsuite/gcc.dg/analyzer/pr101962.c > > @@ -24,7 +24,6 @@ test_1 (void) > > __analyzer_eval (a !=3D NULL); /* { dg-warning "TRUE" } */ > > return *a; /* { dg-line test_1 } */ > > > > - /* { dg-warning "use of uninitialized value '\\*a'" "warning" { > target *-*-* } test_1 } */ > > /* { dg-warning "stack-based buffer over-read" "warning" { target > *-*-* } test_1 } */ > > } > > > > diff --git a/gcc/testsuite/gcc.dg/analyzer/pr109439.c > b/gcc/testsuite/gcc.dg/analyzer/pr109439.c > > new file mode 100644 > > index 00000000000..01c87cf171c > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/analyzer/pr109439.c > > @@ -0,0 +1,12 @@ > > +int would_like_only_oob (int i) > > +{ > > + int arr[] =3D {1,2,3,4,5,6,7}; > > + arr[10] =3D 9; /* { dg-warning "stack-based buffer overflow" } */ > > + arr[11] =3D 15; /* { dg-warning "stack-based buffer overflow" } */ > > + int y1 =3D arr[9]; /* { dg-warning "stack-based buffer over-read" = } */ > > + /* { dg-bogus "use of uninitialized value" "" { target > *-*-* } .-1 } */ > > + > > + arr[18] =3D 15; /* { dg-warning "stack-based buffer overflow" } */ > > + > > + return y1; > > +} > > diff --git a/gcc/testsuite/gcc.dg/analyzer/realloc-5.c > b/gcc/testsuite/gcc.dg/analyzer/realloc-5.c > > index 137e05b87aa..f65f2c6ca25 100644 > > --- a/gcc/testsuite/gcc.dg/analyzer/realloc-5.c > > +++ b/gcc/testsuite/gcc.dg/analyzer/realloc-5.c > > @@ -40,7 +40,6 @@ void test_1 () > > > > /* { dg-warning "UNKNOWN" "warning" { target *-*-* } eval } */ > > /* { dg-warning "heap-based buffer over-read" "warning" { target > *-*-* } eval } */ > > - /* { dg-warning "use of uninitialized value" "warning" { target > *-*-* } eval } */ > > } > > > > free (q); > > -- > > 2.34.1 > > > > --000000000000cdd64c05fda754b3--