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.133.124]) by sourceware.org (Postfix) with ESMTPS id F41C83858D33 for ; Tue, 7 Feb 2023 21:14:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F41C83858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675804440; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=vpORZ9bbX/mUUysw7M196Oy/Pxwbx9M/uN0QsKLMJaA=; b=KZlIroIVrf53gIW052XI9E9FXs4gGSXFwDc67t3nb7ogzgDBfBmfOwdwlnoxjECkHcXpbE Wb/Xx1qKMOWOd0McUiIdFKDKarIHlUAzB87AfToc7xf+IjMjc9KV+YmpJ7+j6eBn9Zj4b5 G+W1ZjoflE0syIGkvJIMV2HIOl28x+U= 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-292-Ij2ZwazmObWU3WiyPxFE8A-1; Tue, 07 Feb 2023 16:13:59 -0500 X-MC-Unique: Ij2ZwazmObWU3WiyPxFE8A-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 40C74811E6E for ; Tue, 7 Feb 2023 21:13:59 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.2.16.227]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1B7B61121314; Tue, 7 Feb 2023 21:13:59 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [pushed] analyzer: fix -Wanalyzer-use-of-uninitialized-value false +ve on "read" [PR108661] Date: Tue, 7 Feb 2023 16:13:57 -0500 Message-Id: <20230207211357.1226071-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,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 List-Id: My integration testing shows many false positives from -Wanalyzer-use-of-uninitialized-value. One cause turns out to be that as of r13-1404-g97baacba963c06 fd_state_machine::on_stmt recognizes calls to "read", and returns true, so that region_model::on_call_post doesn't call handle_unrecognized_call on them, and so the analyzer erroneously "thinks" that the buffer pointed to by "read" is never touched by the "read" call. This works for "fread" because sm-file.cc implements kf_fread, which handles calls to "fread" by clobbering the buffer pointed to. In the long term we should probably be smarter about this and bifurcate the analysis to consider e.g. errors vs full reads vs partial reads, etc (which I'm tracking in PR analyzer/108689). In the meantime, this patch adds a kf_read for "read" analogous to the one for "fread", fixing 6 false positives seen in git-2.39.0 and 2 in haproxy-2.7.1. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r13-5733-gc300e251f5b22d. gcc/analyzer/ChangeLog: PR analyzer/108661 * sm-fd.cc (class kf_read): New. (register_known_fd_functions): Register "read". * sm-file.cc (class kf_fread): Update comment. gcc/testsuite/ChangeLog: PR analyzer/108661 * gcc.dg/analyzer/fread-pr108661.c: New test. * gcc.dg/analyzer/read-pr108661.c: New test. Signed-off-by: David Malcolm --- gcc/analyzer/sm-fd.cc | 33 +++++++++++++++ gcc/analyzer/sm-file.cc | 10 ++++- .../gcc.dg/analyzer/fread-pr108661.c | 40 +++++++++++++++++++ gcc/testsuite/gcc.dg/analyzer/read-pr108661.c | 33 +++++++++++++++ 4 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/fread-pr108661.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/read-pr108661.c diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc index 494d802a1d4..d107390bc00 100644 --- a/gcc/analyzer/sm-fd.cc +++ b/gcc/analyzer/sm-fd.cc @@ -2659,6 +2659,38 @@ private: unsigned m_num_args; }; +/* Handler for "read". + ssize_t read(int fildes, void *buf, size_t nbyte); + See e.g. https://man7.org/linux/man-pages/man2/read.2.html */ + +class kf_read : public known_function +{ +public: + bool matches_call_types_p (const call_details &cd) const final override + { + return (cd.num_args () == 3 + && cd.arg_is_pointer_p (1) + && cd.arg_is_size_p (2)); + } + + /* For now, assume that any call to "read" fully clobbers the buffer + passed in. This isn't quite correct (e.g. errors, partial reads; + see PR analyzer/108689), but at least stops us falsely complaining + about the buffer being uninitialized. */ + void impl_call_pre (const call_details &cd) const final override + { + region_model *model = cd.get_model (); + const svalue *ptr_sval = cd.get_arg_svalue (1); + if (const region *reg = ptr_sval->maybe_get_region ()) + { + const region *base_reg = reg->get_base_region (); + const svalue *new_sval = cd.get_or_create_conjured_svalue (base_reg); + model->set_value (base_reg, new_sval, cd.get_ctxt ()); + } + } +}; + + /* Populate KFM with instances of known functions relating to file descriptors. */ @@ -2672,6 +2704,7 @@ register_known_fd_functions (known_function_manager &kfm) kfm.add ("listen", make_unique ()); kfm.add ("pipe", make_unique (1)); kfm.add ("pipe2", make_unique (2)); + kfm.add ("read", make_unique ()); kfm.add ("socket", make_unique ()); } diff --git a/gcc/analyzer/sm-file.cc b/gcc/analyzer/sm-file.cc index 9cb4e32e286..d99a09b76c4 100644 --- a/gcc/analyzer/sm-file.cc +++ b/gcc/analyzer/sm-file.cc @@ -560,7 +560,11 @@ public: } }; -/* Handler for "fread"". */ +/* Handler for "fread". + size_t fread(void *restrict buffer, size_t size, size_t count, + FILE *restrict stream); + See e.g. https://en.cppreference.com/w/c/io/fread + and https://www.man7.org/linux/man-pages/man3/fread.3.html */ class kf_fread : public known_function { @@ -574,6 +578,10 @@ public: && cd.arg_is_pointer_p (3)); } + /* For now, assume that any call to "fread" fully clobbers the buffer + passed in. This isn't quite correct (e.g. errors, partial reads; + see PR analyzer/108689), but at least stops us falsely complaining + about the buffer being uninitialized. */ void impl_call_pre (const call_details &cd) const final override { region_model *model = cd.get_model (); diff --git a/gcc/testsuite/gcc.dg/analyzer/fread-pr108661.c b/gcc/testsuite/gcc.dg/analyzer/fread-pr108661.c new file mode 100644 index 00000000000..b51cf41ec2a --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/fread-pr108661.c @@ -0,0 +1,40 @@ +typedef __SIZE_TYPE__ size_t; + +extern size_t fread (void *, size_t, size_t, void *); + +struct ring +{ + char buf[1024]; +}; + +int +test_one_large_item (void *fp) +{ + struct ring ring; + int ret; + + ret = fread(&ring, sizeof(ring), 1, fp); + + if (ret != 1) + return 1; + + if (ring.buf[0] > 1) /* { dg-bogus "use of uninitialized value" } */ + return 2; + return 3; +} + +int +test_many_small_items (void *fp) +{ + struct ring ring; + int ret; + + ret = fread(&ring, 1, sizeof(ring), fp); + + if (ret != sizeof(ring)) + return 1; + + if (ring.buf[0] > 1) /* { dg-bogus "use of uninitialized value" } */ + return 2; + return 3; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/read-pr108661.c b/gcc/testsuite/gcc.dg/analyzer/read-pr108661.c new file mode 100644 index 00000000000..70335e6fef1 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/read-pr108661.c @@ -0,0 +1,33 @@ +typedef long int ssize_t; +typedef long unsigned int size_t; + +extern int open(const char* __file, int __oflag, ...) __attribute__((__nonnull__(1))); +extern int close(int __fd); +extern ssize_t read(int __fd, void* __buf, size_t __nbytes); + +struct ring +{ + char buf[1024]; +}; + +int +test(const char* name) +{ + struct ring ring; + int fd; + int ret; + + fd = open(name, 00); + if (fd < 0) + return 0; + + ret = read(fd, &ring, sizeof(ring)); + close(fd); + + if (ret != sizeof(ring)) + return 1; + + if (ring.buf[0] > 1) /* { dg-bogus "use of uninitialized value" } */ + return 2; + return 3; +} -- 2.26.3