Re: [Findutils-patches] [PATCH] Fix file descriptor leaks.

[James Youngman]

On Mon, Mar 29, 2010 at 11:01 PM, Eric Blake <address@hidden> wrote:
> Or you could just update to today's gnulib, where your upstream patch
> doing the same thing was accepted.

Done, in the pushed version.

> I find mid-expression #ifdefs hard to read.  Better is to do this at
> file scope:
>
> #ifndef O_CLOEXEC
> # define O_CLOEXEC 0
> #endif

I've deferred this for now, there are a number of cases like that.

> Cleanup like this is nice, but it might also be nice to split your patch
> into two, to separate unrelated formatting cleanups from the real bug
> fix.

Ack.

>> +#include <sys/resource.h>
>
> Is that universally available?  POSIX requires it, but gnulib does not
> (yet) guarantee it.

I've updated the code (I'll be mailing a patch presently) to avoid
assuming this.


>> +  /* We don't use readdir, because we cannot trust pathconf
>> +   * to tell us the maximum possible length of a path in
>> +   * a given directory (the manpage for readdir_r claims this
>> +   * is the approved method, but the manpage for pathconf indicates
>> +   * that _PC_NAME_MAX is not an upper limit). */
>> +  DIR *dir = opendir_safer (path);
>> +  if (dir)
>> +    {
>> +      int good = 0;
>> +      struct dirent *dent;
>> +
>> +      while ((dent=readdir (dir)) != NULL)
>
> The comment said you don't use readdir, yet here it is.  Bad comment?

Yes, it should have said readdir_r, thanks for spotting this.

> sscanf is not generally safe, because it cannot detect integer
> overflows, but for this particular usage (where the directory name is
> unlikely to overflow), I guess it's okay.

Oh, I'd assumed I'd get ERANGE for that.


> Is anything going to be confused by the fact that the /proc/self/fd
> directory was one of the open fds at the time of your scan?

No, because closedir() is called before we call poll().


> Does extendbuf avoid quadratic reallocation by geometrically growing the
> buffer, rather than just adding 1 element ever iteration?  Although it
> probably doesn't matter for the typical number of open fds.

Yes, and in fact it's in the same directory.   Here's the relevant code:

static size_t
decide_size(size_t current, size_t wanted)
{
  size_t newsize;

  if (0 == current)
    newsize = SIZE_DEFAULT;
  else
    newsize = current;

  while (newsize < wanted)
    {
      if (2 * newsize < newsize)
        xalloc_die ();
      newsize *= 2;
    }
  return newsize;
}

Clearly this could just set newsize=wanted instead of calling
xalloc_die, though there would be a double hit there; the behaviour
would switch to O(N^2) for large N.

>> +void
>> +remember_non_cloexec_fds (void)
>> +{
>> +  int max_fd = get_max_fd ();
>> +  struct remember_fd_context cb_data;
>> +  cb_data.buf = NULL;
>> +  cb_data.used = cb_data.allocated = 0;
>> +
>> +  if (max_fd < INT_MAX)
>> +    ++max_fd;
>> +  visit_open_fds (3, max_fd, remember_fd_if_non_cloexec, &cb_data);
>
> Why are you skipping stdin, stdout, and stderr?  If they start life
> closed, then they are the first ones to be populated by a leaking fd,
> and if you fail to check them, then you'll never notice the leak.

d'oh!  :)


>> +      if (0)
>> +     {
>> +       char * const args[] = {"/bin/ls", "-l", "/proc/self/fd",
>> +                              (char*)NULL };
>> +       execv("/bin/ls", args);
>> +       perror("exec");
>> +     }
>
> Leftover debugging code?

Yes.   More or less deliberately so.   Changing the "if (0)" to "if
(1)" is a simple thing to ask people to do in circumstances where I
can't reproduce their failure.

James.