Re: [PATCH] static analysis support for posix file desccriptor APIs

[David Malcolm <dmalcolm@redhat.com>]

On Thu, 2022-06-23 at 23:58 +0530, Mir Immad wrote:
>  Hi Dave,
> Thanks for the suggestions,
> 
> I changed most of the things that you suggested, however reporting
> for
> warnings like close of known invalid fd was problematic:
> 
> consider the following code:
> 
> if (fd >= 0)
> { write (fd,...); }
> close(fd);
> 
> As I was checking the exploded graph for this; the "close(fd)" stmt
> when
> visited by the FALSE edge of if stmt (fd < 0) finds fd to be in
> m_invalid
> state; hence warns about "close on known invalid fd" which I believe
> is not
> true as fd at that point is not *known* to be invalid. I spent quite
> some
> time on this and decided not to add this diagnosis for now.

That choice seems reasonable to me.

> 
> Also, when close transitions m_invalid to m_close; this leads to
> double
> close even when the second close is outside the ( < 0 ) condition
> which
> again does not seem right.
> if (fd < 0)
> close(fd):
> close(fd); // double close here.

"close" on an invalid FD doesn't give you a closed FD, you still have
an invalid FD.

By analogy with sm-malloc.cc: free (NULL) doesn't make NULL a freed
pointer, it's still NULL, and thus just a no-op.

(although in this case:  close(invalid)  will also set errno to EBADF,
so it's not strictly a no-op).


> 
> > Maybe consolidate on_read and on_write by adding a subroutine that
> > checks for m_closed, and for checking access mode (maybe a
> > "check_for_open_fd" that takes an access mode enum value amongst
> > other
> > params.  If you pass around caller_fndecl, I think much of this
> > code
> > can be shared that way between on_read and on_write (which will
> > help
> > simplify things if we want to support further functions)
> 
> I hope I got this right.
> 
> 
> > > +        }
> > > +    }
> > > +  else
> > > +    {
> >  >+      /* FIXME: add leak reporting */
> 
> > Do you have a testcase that exhibits this behavior?
> 
> I was thinking of the following case:
> void test()
> {
>  open(..);
> }
> Here the resources are leaked because there is no way to free them.

Please add a test case for this, marking it with xfail if need be.

But hopefully it will be easy to implement the FIXME here, with an:
   sm->on_warn (new fd_leak ());
or somesuch (not sure if there's a "tree var" you can give it for this
case though, might have to use NULL).

> 
> In "read" and "write" funcs, I'm warning for unchecked_use_of_fd and
> access_mode_mismatch only when we know fd is not in closed state.
> Otherwise, such code leads to lot of irrelevant warnings, example:
> 
> void test1(const char *path, void *buf) {
>   int fd = open(path, O_RDONLY);
>   if (fd >= 0)
>   {
>   close(fd);
>   read(fd, buf, 1); // read on closed fd + read on possibly invalid
> fd
>   write(fd, buf, 1); // write on closed fd + write on possibly invlid
> fd
>   }
> }

Should we transition the FD to the "stop" state after the first
warning?  That way we ought to avoid a bunch of followup warnings for
that FD.

> 
> 
> Adding docs for new options still remains pending. I added some new
> tests;
> and all tests are passing. The stuff about O_* macros is left as-is.
> 
>  I'm sending a patch in another email.
> 
> Thanks a lot.

Thanks
Dave