Re: [PATCH] find: make pred_empty safer and avoid fd leaks

[Bernhard Voelker]

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