From patchwork Sat Nov 19 00:41:36 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 60873 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2A13C384F4BC for ; Sat, 19 Nov 2022 00:42:11 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 2A13C384F4BC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1668818531; bh=wduqEXlKCv295jZfpsOz+cR61jSx3pehQ2YSxjgwSQ0=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=VjVOSd8jdye8ToaFrm9wSBuZNONWJjq8RFHoxGYHNODc6Flkn+SckjUB64tQJtfLQ /26sGLi1p9EmNvOWcOPg20TphyMGPcPXwX1gXd1Ir5zaf6jtbP8er5RDnp+Yi4RFqR Qq0ZLwmgAaepfB2mFA72BRlk3xCAcbnjOuXOBMVk= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org 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 3A593384F494 for ; Sat, 19 Nov 2022 00:41:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3A593384F494 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-343-EhnTXIUmOHaQbR4RUouNkQ-1; Fri, 18 Nov 2022 19:41:40 -0500 X-MC-Unique: EhnTXIUmOHaQbR4RUouNkQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 357E0101A528 for ; Sat, 19 Nov 2022 00:41:40 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.2.17.189]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0E749C158CF; Sat, 19 Nov 2022 00:41:40 +0000 (UTC) To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [committed] analyzer: fix feasibility false +ve on jumps through function ptrs [PR107582] Date: Fri, 18 Nov 2022 19:41:36 -0500 Message-Id: <20221119004136.3101136-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP 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-Patchwork-Original-From: David Malcolm via Gcc-patches From: David Malcolm Reply-To: David Malcolm Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" PR analyzer/107582 reports a false +ve from -Wanalyzer-use-of-uninitialized-value where the analyzer's feasibility checker erroneously decides that point (B) in the code below is reachable, with "x" being uninitialized there: pthread_cleanup_push(func, NULL); while (ret != ETIMEDOUT) ret = rand() % 1000; /* (A): after the while loop */ if (ret != ETIMEDOUT) x = &z; pthread_cleanup_pop(1); if (ret == ETIMEDOUT) return 0; /* (B): after not bailing out */ due to these contradictionary conditions somehow both holding: * (ret == ETIMEDOUT), at (A) (skipping the initialization of x), and * (ret != ETIMEDOUT), at (B) The root cause is that after the while loop, state merger puts ret in the exploded graph in an UNKNOWN state, and saves the diagnostic at (B). Later, as we explore the feasibilty of reaching the enode for (B), dynamic_call_info_t::update_model is called to push/pop the frames for handling the call to "func" in pthread_cleanup_pop. The "ret" at these nodes in the feasible_graph has a conjured_svalue for "ret", and a constraint on it being either == *or* != ETIMEDOUT. However dynamic_call_info_t::update_model blithely clobbers the model with a copy from the exploded_graph, in which "ret" is UNKNOWN. This patch fixes dynamic_call_info_t::update_model so that it simulates pushing/popping a frame on the model we're working with, preserving knowledge of the constraint on "ret", and enabling the analyzer to "know" that the bail-out must happen. Doing so fixes the false positive. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r13-4158-ga7aef0a5a2b7e2. gcc/analyzer/ChangeLog: PR analyzer/107582 * engine.cc (dynamic_call_info_t::update_model): Update the model by pushing or pop a frame, rather than by clobbering it with the model from the exploded_node's state. gcc/testsuite/ChangeLog: PR analyzer/107582 * gcc.dg/analyzer/feasibility-4.c: New test. * gcc.dg/analyzer/feasibility-pr107582-1.c: New test. * gcc.dg/analyzer/feasibility-pr107582-2.c: New test. Signed-off-by: David Malcolm --- gcc/analyzer/engine.cc | 14 ++++-- gcc/testsuite/gcc.dg/analyzer/feasibility-4.c | 42 ++++++++++++++++++ .../gcc.dg/analyzer/feasibility-pr107582-1.c | 43 +++++++++++++++++++ .../gcc.dg/analyzer/feasibility-pr107582-2.c | 34 +++++++++++++++ 4 files changed, 129 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/feasibility-4.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-2.c diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index b52753da793..db1881cd140 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -2024,16 +2024,22 @@ exploded_node::dump_succs_and_preds (FILE *outf) const /* Implementation of custom_edge_info::update_model vfunc for dynamic_call_info_t. - Update state for the dynamically discorverd calls */ + Update state for a dynamically discovered call (or return), by pushing + or popping the a frame for the appropriate function. */ bool dynamic_call_info_t::update_model (region_model *model, const exploded_edge *eedge, - region_model_context *) const + region_model_context *ctxt) const { gcc_assert (eedge); - const program_state &dest_state = eedge->m_dest->get_state (); - *model = *dest_state.m_region_model; + if (m_is_returning_call) + model->update_for_return_gcall (m_dynamic_call, ctxt); + else + { + function *callee = eedge->m_dest->get_function (); + model->update_for_gcall (m_dynamic_call, ctxt, callee); + } return true; } diff --git a/gcc/testsuite/gcc.dg/analyzer/feasibility-4.c b/gcc/testsuite/gcc.dg/analyzer/feasibility-4.c new file mode 100644 index 00000000000..1a1128089fb --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/feasibility-4.c @@ -0,0 +1,42 @@ +#include "analyzer-decls.h" + +extern int rand (void); + +void test_1 (void) +{ + int ret = 0; + while (ret != 42) + ret = rand() % 1000; + + if (ret != 42) + __analyzer_dump_path (); /* { dg-bogus "path" } */ +} + +static void empty_local_fn (void) {} +extern void external_fn (void); + +void test_2 (void) +{ + void (*callback) () = empty_local_fn; + int ret = 0; + while (ret != 42) + ret = rand() % 1000; + + (*callback) (); + + if (ret != 42) + __analyzer_dump_path (); /* { dg-bogus "path" } */ +} + +void test_3 (void) +{ + void (*callback) () = external_fn; + int ret = 0; + while (ret != 42) + ret = rand() % 1000; + + (*callback) (); + + if (ret != 42) + __analyzer_dump_path (); /* { dg-bogus "path" } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-1.c b/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-1.c new file mode 100644 index 00000000000..15799d02845 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-1.c @@ -0,0 +1,43 @@ +/* { dg-require-effective-target pthread } */ + +#include +#include +#include +#include +#include + +#include "analyzer-decls.h" + +int z; + +static void func(void * o) +{ + (void) o; +} + +int main(int argc, + int ** argv) +{ + struct timespec now; + + int * x; + int ret = 0; + + pthread_cleanup_push(func, NULL); + + while (ret != ETIMEDOUT) + ret = rand() % 1000; + + if (ret != ETIMEDOUT) + x = &z; + + pthread_cleanup_pop(1); + + if (ret == ETIMEDOUT) + return 0; + + __analyzer_dump_path (); /* { dg-bogus "path" } */ + printf("x = %d\n", *x); /* { dg-bogus "use of uninitialized value 'x'" } */ + + return 0; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-2.c b/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-2.c new file mode 100644 index 00000000000..b86ffd86d5c --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/feasibility-pr107582-2.c @@ -0,0 +1,34 @@ +#include +#include + +#include "analyzer-decls.h" + +int z; + +static void func(void) +{ +} + +int main(int argc, + int ** argv) +{ + int * x; + int ret = 0; + void (*callback) () = func; + + while (ret != 110) + ret = rand() % 1000; + + if (ret != 110) + x = &z; + + (*callback) (); + + if (ret == 110) + return 0; + + __analyzer_dump_path (); /* { dg-bogus "path" } */ + printf("x = %d\n", *x); /* { dg-bogus "use of uninitialized value 'x'" } */ + + return 0; +}