From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2209) id CF7B63858D33; Tue, 7 Feb 2023 21:11:27 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CF7B63858D33 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1675804287; bh=Oxot1v50jwDrbfjwqHPLWY6Zacs7HEV3sOtXZIxjApo=; h=From:To:Subject:Date:From; b=qCdtWeZpPy6mqAVItUmvm7MiOIlUqK1icvb/pzeMkMz8RCxvkBfo/ZwPBs4ozpLI/ dF85ElYBqvJF6CFCQ6TVlS19s1TtdNYAnAA4bqdySKAVlaPJrveVTsln1lginLbZm3 UgAt7coNgbfgG6Zd1F3mVpjBJfRldaSuAmKxSsR0= MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: David Malcolm To: gcc-cvs@gcc.gnu.org Subject: [gcc r13-5733] analyzer: fix -Wanalyzer-use-of-uninitialized-value false +ve on "read" [PR108661] X-Act-Checkin: gcc X-Git-Author: David Malcolm X-Git-Refname: refs/heads/master X-Git-Oldrev: c36f3da534e7f411c5bc48c5b6b660e238167480 X-Git-Newrev: c300e251f5b22d15b126f3c643cd55a119994e48 Message-Id: <20230207211127.CF7B63858D33@sourceware.org> Date: Tue, 7 Feb 2023 21:11:27 +0000 (GMT) List-Id: https://gcc.gnu.org/g:c300e251f5b22d15b126f3c643cd55a119994e48 commit r13-5733-gc300e251f5b22d15b126f3c643cd55a119994e48 Author: David Malcolm Date: Tue Feb 7 16:10:20 2023 -0500 analyzer: fix -Wanalyzer-use-of-uninitialized-value false +ve on "read" [PR108661] 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. 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 Diff: --- gcc/analyzer/sm-fd.cc | 33 +++++++++++++++++++++ gcc/analyzer/sm-file.cc | 10 ++++++- gcc/testsuite/gcc.dg/analyzer/fread-pr108661.c | 40 ++++++++++++++++++++++++++ gcc/testsuite/gcc.dg/analyzer/read-pr108661.c | 33 +++++++++++++++++++++ 4 files changed, 115 insertions(+), 1 deletion(-) 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; +}