bugfindutils - Bugs: bug #12044, find still hangs on dead NFS...

 
 

bug #12044: find still hangs on dead NFS filesystems on Solaris

Submitter:  James Youngman <jay>
Submitted:  Sun 20 Feb 2005 08:44:56 AM UTC
   
 
Category:  find Severity:  4 - Important
Item Group:  Wrong result Status:  Fixed
Privacy:  Public Assigned to:  jay
Originator Name:  Martin Buchholz Open/Closed:  Closed
Release:  4.2.18 Fixed Release:  4.2.19
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 04 Mar 2005 12:31:10 AM UTC, comment #18: 

Oden, find doesn't traverse the "whole damn filesystem".  Find version 4.2.18 lstat()s each mount point (see the comments against this bug and also bug #11879 for a complete description).  However, the current CVS code will not even lstat() all the mount points - that is the point of this bug!

Your example "find" command line is wrong, by the way.  I suspect that you meant

find . -type f -name "*beer*"


James Youngman <jay>
Group administrator
Thu 03 Mar 2005 07:37:03 PM UTC, comment #17: 

I don't really understand why "find" needs to traverse the whole damn filesystem with each invocation. I mean if I'm in my home dir and use find from there, why does it stat everything starting from "/" each time?

It just doesn't make sense as I asked it to seach the specified path in current dir. Like so:

cd /home/oden
find . -type -f "*beer*"

Oden Eriksson <oeriksson>
Thu 03 Mar 2005 07:21:20 PM UTC, comment #16: 

Have modified the code to handle the problem where fchdir() fails to return us to the parent directory by issuing a fatal error message.

James Youngman <jay>
Group administrator
Thu 03 Mar 2005 01:03:53 AM UTC, comment #15: 

Hi.

The findutils-nostatmountpt-1.patch patch seems to do the trick for me.

Oden Eriksson <oeriksson>
Wed 02 Mar 2005 10:10:10 PM UTC, comment #14: 

Thanks for the heads-up.  I will change this item from "Fixed" to "remind" to remind me to look at this.

James Youngman <jay>
Group administrator
Tue 01 Mar 2005 09:58:11 PM UTC, comment #13: 

Ouch, safely_chdir_lstat() gets too complicated to be sure it will work as expected.

I see one potential minor issue with findutils-nostatmountpt-1.patch:

In safely_chdir_lstat(), if wd_sanity_check() call fails and RETRY_IF_SANITY_CHECK_FAILS == isfatal, then if fchdir(dotfd) also fails, then you cannot proceed with directory traversal, since you cannot return to proper place from the "wrong" directory.
I don't know when fchdir(dotfd) may fail, though.

Dmitry V. Levin <ldv>
Mon 28 Feb 2005 11:18:24 PM UTC, comment #12: 

Please try the attached patch. 

1. Does it resolve your hanging problem?  
2. Does it behave sensibly if a directory
   changes into a symlink between the lstat()
   and the chdir()?

James Youngman <jay>
Group administrator
Mon 28 Feb 2005 09:34:04 AM UTC, comment #11: 

To recap what remains of our original discussion, we still have one candidate left: -

To chdir() to a subdir, you
- stat("."); save dot_fd;
- st = lstat(file); if ISDIR {
chdir(file);
check stat(".") matches st inode and filesystem.
if not, fchdir(dot_fd) and retry.

Keep track of retry count. Retry at least once, but perhaps
only once.

This is substantially the same as the current implementation, except for the fact that the "retry" takes the place of our existing check of the list of mounted filesystems.  This looks like a promising approach. 

If the chdir() check actually failed because someone is trying to exploit the very race condition that the check exists to detect, then the second cycle of stat-chdir-stat (i.e. the first retry) will then notice that "foo" is now a symbolic link.  This seems to me to be the right thing.

I have a small lingering concern that there may be tests in the predicate list (-prune for example) which would have been true for the old "foo" but now will be false for the new "foo".  Similarly the other way around; some tests that were false may now be true.  Will it be necessary to re-do all the predicates  on the command line which don't have side effects?

James Youngman <jay>
Group administrator
Mon 28 Feb 2005 09:15:24 AM UTC, comment #10: 

Yes, I understand your point, but if you actually go and look at the code, you'll see that it is not true.  check_nofollow() would not fail.  In understanding this problem, you might find it helpful to actually read the code for the check_nofollow() function, as I pointed out in my previous comment.

James Youngman <jay>
Group administrator
Sun 27 Feb 2005 10:29:42 PM UTC, comment #9: 


>Regarding the OS version check, I think it very unlikely that
>if the Matrix fork of Linux was released, it would be based
>on a version of Linux that is now over 7 yeard old. They'd
>fork a more recent version. Hence the example you give is
>spurious. Findutils would not "stop working". If you take a
>look at the check_nofollow() code I'm sure you'll >understand.


My point was that if someone forked the very latest
version of Linux, findutils would work "less well", since
check_nofollow() would fail.  I don't have a fix,
merely pointing out that the current algorithm is
linux-fork-hostile.

Martin

Anonymous
Fri 25 Feb 2005 07:57:49 PM UTC, comment #8: 

Regarding the OS version check, I think it very unlikely that  if the Matrix fork of Linux was released, it would be based on a version of Linux that is now over 7 yeard old.  They'd fork a more recent version.  Hence the example you give is spurious.  Findutils would not "stop working".  If you take a look at the check_nofollow() code I'm sure you'll understand. 

The check for O_NOFOLLOW is done at build time.  It's just that once the program is built it can be installed on to an older box that does not support O_NOFOLLOW.  Hence the obligation to make the check (the alternative being to expose ourselves to race condition security attacks on that machine).

James Youngman <jay>
Group administrator
Thu 24 Feb 2005 06:51:54 AM UTC, comment #7: 


>I can imagine situation where one can subvert subdirectory >with symlink to another subdirectory in the same directory.
>Second check cannot catch this situation.


Very good!  You've found a hole in the stat("..") strategy.
I (Martin Buchholz) hereby retract that suggestion.

Anonymous
Thu 24 Feb 2005 06:47:09 AM UTC, comment #6: 

Regarding the fragility of OS version checks:

It is well accepted now that OS version checks
are a maintenance nightmare.  The "configure revolution"
of relying as much as possible on feature tests, either
at compile time or run time has been a vast improvement.

Compare the convoluted version checks that still litter
emacs configuration code.

One nice free software property is forkability.
If I create Martix, which is identical to Linux
except for the string returned by uname -s,
how many things will stop working or will work less well
in some very subtle way?

Anonymous
Wed 23 Feb 2005 05:41:13 PM UTC, comment #5: 

I like first (chdir/fchdir-back/retry) approach more than second (stat./chdir/stat..).

I can imagine situation where one can subvert subdirectory with symlink to another subdirectory in the same directory.
Second check cannot catch this situation.

Dmitry V. Levin <ldv>
Wed 23 Feb 2005 09:38:04 AM UTC, comment #4: 

I tlike the idea of doing stat("..") after the chdir.  I'l have a think about that to see if it has any hidden gotchas. 

The O_FOLLOW check simply copes with the case where find was compiled on a new system and is being run on a very old system which ignores the flag.  By "very old" I mean Linux kernels released prior to Oct 23 1998.   There is no need to update the check on a yearly basis, because (I hope) support for the O_NOFOLLOW flag won't be removed.

Summary: I will have a think about these alternatives and if any are workable I'll use them.

James Youngman <jay>
Group administrator
Wed 23 Feb 2005 08:07:55 AM UTC, comment #3: 

The current situation of having find hang when any remote
NFS server is unreachable is unacceptable, since it makes
my carefully engineered scripts fail due to the whims of
a flaky network.  I carefully prune PATH, for example,
to have as few dependencies on NFS servers as possible.
Stating the roots of all remote filesystem is even an
invitation to a denial-of-service attack.

It is also unacceptable to erroneously follow symlinks,
since this could lead to unexpected manipulation of
arbitrary files.

What to do?  I still think we can have a robust find.

To chdir() to a subdir, you
- stat("."); save dot_fd;
- st = lstat(file); if ISDIR {
  chdir(file);
  check stat(".") matches st inode and filesystem.
  if not, fchdir(dot_fd) and retry.

Keep track of retry count.  Retry at least once, but perhaps
only once.

Another robust approach is to stat("..") after chdir,
and compare with stat(".") before chdir.

Another comment -- O_NOFOLLOW is very fragile, since
a version check is used, which must be updated on a
regular (yearly?) basis.  I think it might be too early
to use O_NOFOLLOW in production software.

Anonymous
Sun 20 Feb 2005 10:39:03 AM UTC, comment #2: 
James Youngman <jay>
Group administrator
Sun 20 Feb 2005 09:10:50 AM UTC, comment #1: 

The attached patch should solve the problem but only in the case of "find --version".  However, the underlying cause of your diffculty is in effect that the system you built find on currently lacks support for the non-POSIX O_NOFOLLOW flag for open(2).   Find uses this to securely move between directories without following symbolic links.   See bug #11879 for a background on the use of O_FOLLOW. 

You can find out if find thinks O_NOFOLLOW works for you by checking the output of "find --version" (you will have to apply this patch first of course if your NFS server still isn't working).   O_NOFOLLOW appears in the feature list if find was built on a system which defines it, but it is disabled if find think it is actually running on a system that ignores the flag (all systems except the GNU/Linux and FreeBSD get the benefit of the doubt at the moment).

This issue is discussed in quite some detail in the "Security Considerations" section of the findutils manual (the manual is included in the source distribution).  See for example the section entitled "Changing the Current Working Directory".

With the code as it stands, there is a trade-off to be made between

1. Security (i.e. not being exposed to race conditions)
2. Reliability (i.e. not being fatally confused by automount)
3. Convenience (i.e. stil working on clients of unreachable NFS servers)

As noted in bug #11879, in the absence of O_NOFOLLOW I don't know of a way to solve all of these problems simultaneously.  Without O_NOFOLLOW we can solve any two of those problems, but not all three.

What it comes down to is that we need to find a way of changing directory on Solaris which either does't follow symbolic links or can tell when this has happened without being fooled by automounters.

James Youngman <jay>
Group administrator
Sun 20 Feb 2005 08:44:56 AM UTC, original submission:  

(From Martin Buchholz ...)

I still see hangs in find 4.2.18, despite the
ChangeLog entry that suggests this has been fixed.

Even find --version hangs with this stacktrace

(dbx) where
[1] _lstat64(0x4, 0xffbff160, 0x1, 0xfeabbe60, 0x81010100, 0xff00), at 0xfeb1abac
=>[2] optionp_stat(name = 0x7a548 "/java/tl", p = 0xffbff160), line 229 in "find.c"
[3] set_fstype_devno(p = 0x79b58), line 182 in "fstype.c"
[4] get_mounted_devices(n = 0x71114), line 278 in "fstype.c"
[5] init_mounted_dev_list(), line 668 in "find.c"
[6] main(argc = 2, argv = 0xffbff3c4), line 448 in "find.c"

where /java/tl is the remotely mounted filesystem that
is "down" during command execution.

This is pretty serious. In a large networked environment,
there is always some NFS server that is broken or very slow
at the moment.

The --version flag at the very least should be handled
before trying to "do" any real work.


James Youngman <jay>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #2632:  findutils-nostatmountpt-1.patch added by jay (9KiB - text/x-patch - Proposed fix #1)
file #2596:  findutils-4.2.18-reposition-init_mounted_dev_list.patch added by jay (3KiB - text/x-patch - Patch to make find --version work, at least.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

Only logged-in users can vote.

 

Follow 10 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2005-03-07 jay Open/ClosedOpen Closed
    Fixed ReleaseNone 4.2.19
2005-03-03 jay StatusRemind Fixed
2005-03-02 jay StatusFixed Remind
2005-03-01 jay StatusNeed Info Fixed
2005-02-28 jay Attached File- Added findutils-nostatmountpt-1.patch, #2249
2005-02-28 jay StatusNone Need Info
2005-02-23 ldv Carbon-Copy- Added ldv
2005-02-20 jay Severity3 - Normal 4 - Important
    Attached File- Added findutils-4.2.18-reposition-init_mounted_dev_list.patch, #2218

Back to the top

Powered by Savane 3.13-d3ae.
Corresponding source code