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
