From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52d.google.com (mail-pg1-x52d.google.com [IPv6:2607:f8b0:4864:20::52d]) by sourceware.org (Postfix) with ESMTPS id 69B4A3857357; Thu, 8 Jun 2023 18:18:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 69B4A3857357 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-x52d.google.com with SMTP id 41be03b00d2f7-5428f63c73aso519243a12.1; Thu, 08 Jun 2023 11:18:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686248317; x=1688840317; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=qlSOGr3bfqDoiUms1ddTpBxVxplDIvhqTxn1AOnmQtM=; b=E2Hirn7Q6+sJb/DQnox39mglVr51W1KNRhXNVC/iQI/zLMFTSn9KOFhuIkl1llJ2CY cqp0frdO+g+R5InMXyXk0mcTLwDx8VCg3aLIEXvl1r3AN0qvsq9L/7UnfxzE6Gh3mYWW vwrRw4UJN5WZ+JrDNrkri5isiS1r8AASPSO/E47XMVnIkVhGAES/EtmpSK8QqD6+SpNJ Gd2jHQBkLtwry342DW3jTnCO8u6eEqw1su1WspQeHb91uvdP87+6cI4xgYl1qeDoK/RB X0GJ9p8IW7A5TnK/CW/8zzjMnq4MQRYSYtssjTjWvBwNerh9mzchy+ld2mD6+BQJK6CF pwxg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686248317; x=1688840317; 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=qlSOGr3bfqDoiUms1ddTpBxVxplDIvhqTxn1AOnmQtM=; b=lRdZKZ7OwEys54XiJlAhcZaKVScZnDIpOLXFepGf8yTc9JalUDKIiYVuIi1ps93mwp mRzje7NHIlAnDlJrXK8jRa9TUsuzwzSuLM+jm+pExOsBNUOOWiDPfOCDRcCvMlWzfUAI AMcpvT/Zb7R4AsKqJGlyeiKooX/3AI2vGaCohdk+6JT4LL4iBh0G2KC0+H9nYCkQbZtD wDDh9As3q9geYOR3b+wsLs7UdRPXHLMKuYorRssXMCU0GC/IcZldkoMul9q1c2/Xnwsb J1Fus4chSt+L1d19bBtdvrh4EG9kWUEVphpEhkUMyrV5yiDmfhToenq163tz0EmSOhDF bZxw== X-Gm-Message-State: AC+VfDzPwJZEovbdHFjFrQPBc9xCaRMhvVbcXBMVPIjdE1vkAUGUR1YG ZKOufX3nZOpqO4VHJP6eXVQDT8x2oc7iCBZomxYxGNP8UpCM X-Google-Smtp-Source: ACHHUZ50xBU8KbXivWDunuvIr2bQjtOMTPZ+YQNG5QDDhzj+FBpFm82asE7iAEmgrKHPy7irXBL2Kk15gzin4/3qREY= X-Received: by 2002:a17:90a:540c:b0:253:45ce:fad7 with SMTP id z12-20020a17090a540c00b0025345cefad7mr5943971pjh.31.1686248316917; Thu, 08 Jun 2023 11:18:36 -0700 (PDT) MIME-Version: 1.0 References: <20230606114858.447221-1-vultkayn@gcc.gnu.org> <225A7AAD-E31D-4BF1-AC1F-CE160989FE60@linaro.org> In-Reply-To: <225A7AAD-E31D-4BF1-AC1F-CE160989FE60@linaro.org> From: Benjamin Priour Date: Thu, 8 Jun 2023 20:18:25 +0200 Message-ID: Subject: Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439] To: Maxim Kuvyrkov Cc: gcc-patches@gcc.gnu.org, Benjamin Priour , dmalcolm@redhat.com Content-Type: multipart/alternative; boundary="000000000000eedf4e05fda24770" X-Spam-Status: No, score=-7.3 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: --000000000000eedf4e05fda24770 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 > > > > --000000000000eedf4e05fda24770--