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 EF638385840C for ; Fri, 29 Jul 2022 23:54:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EF638385840C Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-228-9Eq-xzWKMH63PDMicLOgtA-1; Fri, 29 Jul 2022 19:54:40 -0400 X-MC-Unique: 9Eq-xzWKMH63PDMicLOgtA-1 Received: by mail-qt1-f197.google.com with SMTP id a10-20020ac84d8a000000b0031ee6389b7eso3682885qtw.6 for ; Fri, 29 Jul 2022 16:54:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=ZMxrmRbG9uszFXcPmp7elhWPLcpmKCrC6VEN36bjc4o=; b=6GDqMHaK8TwActooCxbCKztI9a+GBq/HNVo194MkyQYw1Lrbej+rDFxsrjavQm/2nL xN6WVpS25qc0zQ9DN+TPbklXse4NV4/XGGbmKZatAlTAR6IqJljTSq1jxNbesVzM1cdZ ll/ZytkW92SQ7hya9tHmfGqiINjusY74Udf2vJZlpWn5DX1jJa2oNY1o/h3n56aZ+QYd uEbRQZB5xkPyU+7GWJX4S35s6NwFFx+DrDEIaRKDWv82mNjY7JVdSDDdTyFJdSU5CX1B 2JuuMZfgLZhTkqkqeMv3SC56XwpLwUZu9LGt2dvAuBqTiKmsLC1nvlQTqEXx8ByhMjg9 QQLw== X-Gm-Message-State: ACgBeo1w3borIxw7zBMoyqN/pzhjaBOQtcieoe2Mzszyr6LcJUxB7wji KQmHG0FxsRbp06T6byUG7sm+jNk+pLS8lbYIEYmz7EzS/O/KhkRKrY+aaurYi/FTSrq5i3uHvuo gc6FJYKoHFzKBnUdJ1w== X-Received: by 2002:ad4:5ce8:0:b0:473:d3c0:5c2d with SMTP id iv8-20020ad45ce8000000b00473d3c05c2dmr5208944qvb.83.1659138879824; Fri, 29 Jul 2022 16:54:39 -0700 (PDT) X-Google-Smtp-Source: AA6agR6r8T3Cd00Eatud4CefdXWgOEXvTZVjnaYPAZc7tuEgLfwc4hJ0IiJTefO5CKdALOiwVjOSSg== X-Received: by 2002:ad4:5ce8:0:b0:473:d3c0:5c2d with SMTP id iv8-20020ad45ce8000000b00473d3c05c2dmr5208937qvb.83.1659138879557; Fri, 29 Jul 2022 16:54:39 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id r11-20020ae9d60b000000b006b5869c1525sm3479961qkk.21.2022.07.29.16.54.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Jul 2022 16:54:38 -0700 (PDT) Message-ID: <7d6ecd3de29ec5b5cd515597b99f3dacf2426b49.camel@redhat.com> Subject: Re: [PATCH] analyzer: support for creat, dup, dup2 and dup3 in sm-fd.cc [PR106300] From: David Malcolm To: mirimnan017@gmail.com, gcc-patches@gcc.gnu.org Cc: Immad Mir Date: Fri, 29 Jul 2022 19:54:37 -0400 In-Reply-To: References: User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, 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-List-Received-Date: Fri, 29 Jul 2022 23:54:45 -0000 On Fri, 2022-07-29 at 21:59 +0530, Immad Mir wrote: > This patch extends the state machine in sm-fd.cc to support > creat, dup, dup2 and dup3 functions. Thanks for the patch. Please can you use PR 106298 for this (in the ChangeLog and subject line), rather than 106300, as it's more specific. It's always a battle to keep the subject line a reasonable length, especially with an "analyzer: " prefix and a " [PRnnnnnn]" suffix. I think you can leave off the " in sm-fd.cc" from the subject line, which will help. I think the patch is close to ready; review comments inline... > > Lightly tested on x86_64 Linux. > > gcc/analyzer/ChangeLog: >         PR analyzer/106300 >         * sm-fd.cc (fd_state_machine::on_open): Add >         creat, dup, dup2 and dup3 functions. >         (enum dup): New. >         (fd_state_machine::valid_to_unchecked_state): New. >         (fd_state_machine::on_creat): New. >         (fd_state_machine::on_dup): New. > > gcc/testsuite/ChangeLog: >         PR analyzer/106300 >         * gcc.dg/analyzer/fd-1.c: Add tests for 'creat'. >         * gcc.dg/analyzer/fd-2.c: Likewise. >         * gcc.dg/analyzer/fd-4.c: Likewise. >         * gcc.dg/analyzer/fd-6.c: New tests. At some point we'll want to add documentation to invoke.texi about what functions we're special-casing in -fanalyzer, but you can postpone the sm-fd.cc part of that to a follow-up patch if you like. [...snip...] > --- a/gcc/analyzer/sm-fd.cc > +++ b/gcc/analyzer/sm-fd.cc > @@ -69,6 +69,13 @@ enum access_directions >    DIRS_WRITE >  }; >   > +enum dup > +{ > +  DUP_1, > +  DUP_2, > +  DUP_3 > +}; Please add a comment documenting this enum. [...snip...] > +void > +fd_state_machine::check_for_dup (sm_context *sm_ctxt, const > supernode *node, > +                                const gimple *stmt, const gcall > *call, > +                                const tree callee_fndecl, enum dup > kind) const > +{ > +  tree lhs = gimple_call_lhs (call); > +  tree arg_1 = gimple_call_arg (call, 0); > +  state_t state_arg_1 = sm_ctxt->get_state (stmt, arg_1); > +  tree diag_arg_1 = sm_ctxt->get_diagnostic_tree (arg_1); > +  if (state_arg_1 == m_stop) > +    return; > +  if (!(is_constant_fd_p (state_arg_1) || is_valid_fd_p > (state_arg_1))) > +    { > +      sm_ctxt->warn ( > +         node, stmt, arg_1, > +         new fd_use_without_check (*this, diag_arg_1, > callee_fndecl)); > +      if (kind == DUP_1) > +       return; > +    } I don't see test coverage for dup of a closed fd; I think we'd want to warn for that with an fd_use_after_close. That ought to be tested for here, but if I'm reading it right, the check is missing. Can you reuse fd_state_machine::check_for_open_fd here, rather than duplicating the logic for use-without-check and for use-after-close ? (since I think the code is almost the same, apart, perhaps from that early return when kind is DUP_1). > +  switch (kind) > +    { > +    case DUP_1: > +      if (!is_constant_fd_p (state_arg_1)) > +       if (lhs) > +         sm_ctxt->set_next_state (stmt, lhs, > +                                  valid_to_unchecked_state > (state_arg_1)); > +      break; What happens on dup() of an integer constant? My understanding is that: * in terms of POSIX: dup will return either a new FD, or -1. * as for this patch, the LHS won't be checked (for validity, or leaks). Shouldn't we transition the LHS to m_unchecked_read_write? Or am I missing something? I don't see test coverage for dup of integer constants - please add some. > + > +    case DUP_2: > +    case DUP_3: > +      tree arg_2 = gimple_call_arg (call, 1); > +      state_t state_arg_2 = sm_ctxt->get_state (stmt, arg_2); > +      tree diag_arg_2 = sm_ctxt->get_diagnostic_tree (arg_2); > +      if (state_arg_2 == m_stop) > +       return; > +      if (!(is_constant_fd_p (state_arg_2) || is_valid_fd_p > (state_arg_2))) > +       { > +         sm_ctxt->warn ( > +             node, stmt, arg_2, > +             new fd_use_without_check (*this, diag_arg_2, > callee_fndecl)); > +         return; > +       } > + > +      if (!is_constant_fd_p (state_arg_2)) > +       { > +         if (lhs) > +           sm_ctxt->set_next_state (stmt, lhs, > +                                    valid_to_unchecked_state > (state_arg_2)); I got a bit confused by the logic here, but I think it's correct. Maybe add a comment clarifying that we want to make sure we don't pass -1 as the 2nd argument, and therefor we want to check for is_valid_fd_p. We're not yet modeling that lhs == arg_2 on success, but maybe we don't need to. Maybe add a comment to that effect. [...snip...] > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c I get a bit lost when there are lots of numbered test files. Perhaps you could renaming this, say, to fd-dup-1.c, to reflect the theme of what's being tested. But that's up to you. [...snip...] Thanks again for the patch. Dave