[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Findutils-patches] [PATCH] Fix file descriptor leaks.
From: |
James Youngman |
Subject: |
Re: [Findutils-patches] [PATCH] Fix file descriptor leaks. |
Date: |
Tue, 30 Mar 2010 01:16:22 +0100 |
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.