Re: [PATCH] analyzer: support for creat, dup, dup2 and dup3 in sm-fd.cc [PR106300]

[David Malcolm <dmalcolm@redhat.com>]

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