From patchwork Tue Aug 22 01:21:27 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 136439 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b82d:0:b0:3f2:4152:657d with SMTP id z13csp3348395vqi; Mon, 21 Aug 2023 18:24:53 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG6idOcwOb9uC4+aJH0aFv6e6OzL+9hNi9pcVXzMlaIx89MyzjMqHIcyo3QGtgudqF0qvxP X-Received: by 2002:aa7:db47:0:b0:523:b37e:b83b with SMTP id n7-20020aa7db47000000b00523b37eb83bmr8464210edt.13.1692667493374; Mon, 21 Aug 2023 18:24:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1692667493; cv=none; d=google.com; s=arc-20160816; b=AhLg7oFuEB+QmqwSlLIWTNtFWojvDN9t+AzBN0E8UTljYmT6y74LTS8pfapdkM1daZ GhWOskF/2lQ9JRBQT1dUIChYl3r5a/MLSmYm9dXxKor1G4knKKHtMpV8IVFveNnRvmNH by6IKkezsELctqaYhtYcOCSLGAWei9TMpf27PVk5L1RapVcHQIfZvjoppi9AL8OGwbxf 9uZRjvFtlT8z8SZJwHs1o97thjZjsFCWlOfpEsal2qRNgkFZGwHsCIgIuSxJJklum7HL 2bHq8dzPRGH7Tme9SCucuAjwEKzXZYwTj/iHhGtfDD2fHQsNeQV3cAq8hR6P0cPMHEu2 zu5A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:from:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence :content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:dmarc-filter:delivered-to :dkim-signature:dkim-filter; bh=ASRZ5vyk517leCGoXgz7Y0PPc8KsX/uEjo1fB6DdXeY=; fh=fa0YJSH7y4k0snNuxsKgOI6vGP7j2f0jR9F1WHC95UA=; b=ZnXRk1BYibThWPao2LinNQxCTAhvoK3/yh4sacLMdyRAPzp7OCu5Fis6/B5ATRwhd/ qr2gyvZzWpdZcaJ07Bd/w567isjrRZw9SQUL6Gd12XgYi5xsuAZdHwblQVVNexkzR0g6 WCDN1ObuB9OaPTVt7vJSYl87bpEnYZphIIsh5Wh2uVSx6RCGdpLwP8jcuipReuPiaMK9 Px3ZiAQeGXz9IU7A/uQi2Q7uTGSO8pfx6rfxdSPTyTcr15g4lelAk9UxlUB7cMNIY9mR TuH8Szh8H8Z6moQGBN9M0BKAGROj576tS/jEb7fJeMVTdHymrPZpI/rkqP61qdxsFdO9 lGUQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=toyXVsLj; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (ip-8-43-85-97.sourceware.org. [8.43.85.97]) by mx.google.com with ESMTPS id i18-20020aa7c712000000b0052570917661si6543171edq.615.2023.08.21.18.24.53 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 21 Aug 2023 18:24:53 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) client-ip=8.43.85.97; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=toyXVsLj; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 6F3F8382DC71 for ; Tue, 22 Aug 2023 01:23:31 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6F3F8382DC71 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1692667411; bh=ASRZ5vyk517leCGoXgz7Y0PPc8KsX/uEjo1fB6DdXeY=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=toyXVsLjXQxtwGiVri1pqDHCEEv3ufde9PLEqyPWGeakv/wSr1jc0Wl8J08P0WwAH 1qaLGlqPCDhLBrDetsm3EtPdRwY1dDUIN1T3wlckP7p961JXPHz/1jx7DkAcrWXKyC G+8FKfgb2i5Of0Ti90bWimd4hMVtYd/7VswhuVcU= 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.133.124]) by sourceware.org (Postfix) with ESMTPS id 584363858409 for ; Tue, 22 Aug 2023 01:21:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 584363858409 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-36-epLIzax-MLKPVKagzoO9gw-1; Mon, 21 Aug 2023 21:21:31 -0400 X-MC-Unique: epLIzax-MLKPVKagzoO9gw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4E4B3858EED for ; Tue, 22 Aug 2023 01:21:31 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.22.16.93]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2C27A40D2843; Tue, 22 Aug 2023 01:21:31 +0000 (UTC) To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [pushed 6/6] analyzer: check format strings for null termination [PR105899] Date: Mon, 21 Aug 2023 21:21:27 -0400 Message-Id: <20230822012127.2817996-6-dmalcolm@redhat.com> In-Reply-To: <20230822012127.2817996-1-dmalcolm@redhat.com> References: <20230822012127.2817996-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 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_H4, RCVD_IN_MSPIKE_WL, 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+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1774890509474717694 X-GMAIL-MSGID: 1774890509474717694 This patch extends -fanalyzer to check the format strings of calls to functions marked with '__attribute__ ((format...))'. The only checking done in this patch is to check that the format string is a valid null-terminated string; this patch doesn't attempt to check the content of the format string. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r14-3376-g3b691e0190c6e7. gcc/analyzer/ChangeLog: PR analyzer/105899 * call-details.cc (call_details::call_details): New ctor. * call-details.h (call_details::call_details): New ctor decl. (struct call_arg_details): Move here from region-model.cc. * region-model.cc (region_model::check_call_format_attr): New. (region_model::check_call_args): Call it. (struct call_arg_details): Move it to call-details.h. * region-model.h (region_model::check_call_format_attr): New decl. gcc/testsuite/ChangeLog: PR analyzer/105899 * gcc.dg/analyzer/attr-format-1.c: New test. * gcc.dg/analyzer/sprintf-1.c: Update expected results for now-passing tests. --- gcc/analyzer/call-details.cc | 10 ++ gcc/analyzer/call-details.h | 30 +++++ gcc/analyzer/region-model.cc | 125 +++++++++++++----- gcc/analyzer/region-model.h | 2 + gcc/testsuite/gcc.dg/analyzer/attr-format-1.c | 31 +++++ gcc/testsuite/gcc.dg/analyzer/sprintf-1.c | 6 +- 6 files changed, 172 insertions(+), 32 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/attr-format-1.c diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call-details.cc index e497fc58e028..8f5b28ce6c26 100644 --- a/gcc/analyzer/call-details.cc +++ b/gcc/analyzer/call-details.cc @@ -58,6 +58,16 @@ call_details::call_details (const gcall *call, region_model *model, } } +/* call_details's ctor: copy CD, but override the context, + using CTXT instead. */ + +call_details::call_details (const call_details &cd, + region_model_context *ctxt) +{ + *this = cd; + m_ctxt = ctxt; +} + /* Get the manager from m_model. */ region_model_manager * diff --git a/gcc/analyzer/call-details.h b/gcc/analyzer/call-details.h index 86f0e68072bd..58b5ccd2acde 100644 --- a/gcc/analyzer/call-details.h +++ b/gcc/analyzer/call-details.h @@ -30,6 +30,7 @@ class call_details public: call_details (const gcall *call, region_model *model, region_model_context *ctxt); + call_details (const call_details &cd, region_model_context *ctxt); region_model *get_model () const { return m_model; } region_model_manager *get_manager () const; @@ -83,6 +84,35 @@ private: const region *m_lhs_region; }; +/* A bundle of information about a problematic argument at a callsite + for use by pending_diagnostic subclasses for reporting and + for deduplication. */ + +struct call_arg_details +{ +public: + call_arg_details (const call_details &cd, unsigned arg_idx) + : m_call (cd.get_call_stmt ()), + m_called_fndecl (cd.get_fndecl_for_call ()), + m_arg_idx (arg_idx), + m_arg_expr (cd.get_arg_tree (arg_idx)) + { + } + + bool operator== (const call_arg_details &other) const + { + return (m_call == other.m_call + && m_called_fndecl == other.m_called_fndecl + && m_arg_idx == other.m_arg_idx + && pending_diagnostic::same_tree_p (m_arg_expr, other.m_arg_expr)); + } + + const gcall *m_call; + tree m_called_fndecl; + unsigned m_arg_idx; // 0-based + tree m_arg_expr; +}; + } // namespace ana #endif /* GCC_ANALYZER_CALL_DETAILS_H */ diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 0fce18896fbc..99817aee3a93 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -1271,14 +1271,108 @@ region_model::on_stmt_pre (const gimple *stmt, } } +/* Given a call CD with function attribute FORMAT_ATTR, check that the + format arg to the call is a valid null-terminated string. */ + +void +region_model::check_call_format_attr (const call_details &cd, + tree format_attr) const +{ + /* We assume that FORMAT_ATTR has already been validated. */ + + /* arg0 of the attribute should be kind of format strings + that this function expects (e.g. "printf"). */ + const tree arg0_tree_list = TREE_VALUE (format_attr); + if (!arg0_tree_list) + return; + + /* arg1 of the attribute should be the 1-based parameter index + to treat as the format string. */ + const tree arg1_tree_list = TREE_CHAIN (arg0_tree_list); + if (!arg1_tree_list) + return; + const tree arg1_value = TREE_VALUE (arg1_tree_list); + if (!arg1_value) + return; + + unsigned format_arg_idx = TREE_INT_CST_LOW (arg1_value) - 1; + if (cd.num_args () <= format_arg_idx) + return; + + /* Subclass of annotating_context that + adds a note about the format attr to any saved diagnostics. */ + class annotating_ctxt : public annotating_context + { + public: + annotating_ctxt (const call_details &cd, + unsigned fmt_param_idx) + : annotating_context (cd.get_ctxt ()), + m_cd (cd), + m_fmt_param_idx (fmt_param_idx) + { + } + void add_annotations () final override + { + class reason_format_attr + : public pending_note_subclass + { + public: + reason_format_attr (const call_arg_details &arg_details) + : m_arg_details (arg_details) + { + } + + const char *get_kind () const final override + { + return "reason_format_attr"; + } + + void emit () const final override + { + inform (DECL_SOURCE_LOCATION (m_arg_details.m_called_fndecl), + "parameter %i of %qD marked as a format string" + " via %qs attribute", + m_arg_details.m_arg_idx + 1, m_arg_details.m_called_fndecl, + "format"); + } + + bool operator== (const reason_format_attr &other) const + { + return m_arg_details == other.m_arg_details; + } + + private: + call_arg_details m_arg_details; + }; + + call_arg_details arg_details (m_cd, m_fmt_param_idx); + add_note (make_unique (arg_details)); + } + private: + const call_details &m_cd; + unsigned m_fmt_param_idx; + }; + + annotating_ctxt my_ctxt (cd, format_arg_idx); + call_details my_cd (cd, &my_ctxt); + my_cd.check_for_null_terminated_string_arg (format_arg_idx); +} + /* Ensure that all arguments at the call described by CD are checked - for poisoned values, by calling get_rvalue on each argument. */ + for poisoned values, by calling get_rvalue on each argument. + + Check that calls to functions with "format" attribute have valid + null-terminated strings for their format argument. */ void region_model::check_call_args (const call_details &cd) const { for (unsigned arg_idx = 0; arg_idx < cd.num_args (); arg_idx++) cd.get_arg_svalue (arg_idx); + + /* Handle attribute "format". */ + if (tree format_attr = cd.lookup_function_attribute ("format")) + check_call_format_attr (cd, format_attr); } /* Update this model for an outcome of a call that returns a specific @@ -3175,35 +3269,6 @@ region_model::set_value (tree lhs, tree rhs, region_model_context *ctxt) set_value (lhs_reg, rhs_sval, ctxt); } -/* A bundle of information about a problematic argument at a callsite - for use by pending_diagnostic subclasses for reporting and - for deduplication. */ - -struct call_arg_details -{ -public: - call_arg_details (const call_details &cd, unsigned arg_idx) - : m_call (cd.get_call_stmt ()), - m_called_fndecl (cd.get_fndecl_for_call ()), - m_arg_idx (arg_idx), - m_arg_expr (cd.get_arg_tree (arg_idx)) - { - } - - bool operator== (const call_arg_details &other) const - { - return (m_call == other.m_call - && m_called_fndecl == other.m_called_fndecl - && m_arg_idx == other.m_arg_idx - && pending_diagnostic::same_tree_p (m_arg_expr, other.m_arg_expr)); - } - - const gcall *m_call; - tree m_called_fndecl; - unsigned m_arg_idx; // 0-based - tree m_arg_expr; -}; - /* Issue a note specifying that a particular function parameter is expected to be a valid null-terminated string. */ diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 63a67b35350b..3979bf124783 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -597,6 +597,8 @@ private: region_model_context *ctxt) const; void check_call_args (const call_details &cd) const; + void check_call_format_attr (const call_details &cd, + tree format_attr) const; void check_external_function_for_access_attr (const gcall *call, tree callee_fndecl, region_model_context *ctxt) const; diff --git a/gcc/testsuite/gcc.dg/analyzer/attr-format-1.c b/gcc/testsuite/gcc.dg/analyzer/attr-format-1.c new file mode 100644 index 000000000000..c7fa705585d7 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/attr-format-1.c @@ -0,0 +1,31 @@ +extern int +my_printf (void *my_object, const char *my_format, ...) + __attribute__ ((format (printf, 2, 3))); +/* { dg-message "parameter 2 of 'my_printf' marked as a format string via 'format' attribute" "attr note" { target *-*-* } .-2 } */ +/* { dg-message "argument 2 of 'my_printf' must be a pointer to a null-terminated string" "arg note" { target *-*-* } .-3 } */ + +int test_empty (void *my_object, const char *msg) +{ + return my_printf (my_object, ""); +} + +int test_percent_s (void *my_object, const char *msg) +{ + return my_printf (my_object, "%s\n", msg); +} + +int +test_unterminated_format (void *my_object) +{ + char fmt[3] = "abc"; + return my_printf (my_object, fmt); /* { dg-warning "stack-based buffer over-read" } */ + /* { dg-message "while looking for null terminator for argument 2 \\('&fmt'\\) of 'my_printf'..." "event" { target *-*-* } .-1 } */ +} + +int +test_uninitialized_format (void *my_object) +{ + char fmt[10]; + return my_printf (my_object, fmt); /* { dg-warning "use of uninitialized value 'fmt\\\[0\\\]'" } */ + /* { dg-message "while looking for null terminator for argument 2 \\('&fmt'\\) of 'my_printf'..." "event" { target *-*-* } .-1 } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c b/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c index c79525d912f1..f8dc806d6192 100644 --- a/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c @@ -53,12 +53,14 @@ int test_uninit_fmt_buf (char *dst) { const char fmt[10]; - return sprintf (dst, fmt); // TODO (PR analyzer/105899): complain about "fmt" not being initialized + return sprintf (dst, fmt); /* { dg-warning "use of uninitialized value 'fmt\\\[0\\\]'" } */ + /* { dg-message "while looking for null terminator for argument 2 \\('&fmt'\\) of 'sprintf'..." "event" { target *-*-* } .-1 } */ } int test_fmt_not_terminated (char *dst) { const char fmt[3] = "foo"; - return sprintf (dst, fmt); // TODO (PR analyzer/105899): complain about "fmt" not being terminated + return sprintf (dst, fmt); /* { dg-warning "stack-based buffer over-read" } */ + /* { dg-message "while looking for null terminator for argument 2 \\('&fmt'\\) of 'sprintf'..." "event" { target *-*-* } .-1 } */ }