[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Findutils-patches] [PATCH] find: make pred_empty safer and avoid fd
From: |
Bernhard Voelker |
Subject: |
Re: [Findutils-patches] [PATCH] find: make pred_empty safer and avoid fd leaks |
Date: |
Wed, 20 Feb 2019 09:00:46 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.0 |
On 2/19/19 6:38 PM, James Youngman wrote:
> Thanks for this fix.
>
> But, I think the the code I originally I wrote here is broken in a way
> that's not completely fixed by your patch. The case is where the
> type of a file changes between directory and non-directory. The code
> as it is now will issue an error whose message corresponds to ENOTDIR.
> If a directory is replaced by an empty file, I think there'a an
> argument for returning true without issuing a diagnostic (and hence
> without having find return with a nonzero exit status).
This is true: with extreme code like ...
int main (int argc, char**argv) {
char const *F = "xxx";
while (1) {
close (open(F, O_CREAT|O_EXCL|O_CLOEXEC,
S_IWUSR|S_IRUSR|S_IRGRP|S_IROTH));
unlink (F);
mkdir (F, S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);
rmdir (F);
}
return 0;
}
... running in parallel, I could trigger an error message in every
2nd to 10th execution of 'find'.
However, in normal life, I've never seen an issue with -empty yet
nor did we receive any bug report about it. I just recognized
the FD leaks when reading the code (again), so I fixed that right
away.
> On the assumption that you also think this race should be handled
> silently, how about this approach?
>
> Instead of keeping the pre-existing race introduced by the iniail
> S_ISDIR check, we could begin by open()ing the file, without
> O_DIRECTORY. If the open fails, issue an error and return.
> Otherwise, perform fstat on the fd. If the fstat fails, issue an
> error message and return false. Otherwise, if the file is a regular
> file (S_ISREG, as current code) and it is zero bytes long, return
> true. Otherwise, if it is a non-directory, return false (as now).
> Last, handle the directory case. Perform fdopendir (which, now, we
> know cannot fail with ENOTDIR) and attempt readdir on it. If any
> readdir returns non-NULL, return false.
>
> If readdir returns NULL with errno != 0, I think it is probably safest
> to issue an error and return false, as now. Except EOVERFLOW: for
> this, we should issue no diagnostic and simply return false (since we
> know there is something in the directory, even though it cannot be
> correctly represented). Otherwise (that is, readdir returns NULL
> with errno == 0 and we didn't previously get a non-NULL result) return
> true.
>
> What do you think?
Thanks, the idea is nice, however, I see the following problems:
a) open()ing also all non-dir files would come with a big performance
penalty. As we already have stat()ed the file, we already know if
it is empty (or was at that time).
I assume that users have already become accustomed to using -empty
over "-size 0c", so they'd be wondering about a slowdown.
b) open() will already fail if a go-r file is owned by another user.
(The same applies to directories, but for these we cannot determine
if it is empty anyway.)
I think this is a blocker for the idea.
As written in the docs, 'find' will always have to fight with races.
For me, the question in this -empty case is more whether we should issue
an error diagnostic or not.
Have a nice day,
Berny