bugfindutils - Bugs: bug #11879, all mount-points lstat()-ed, but...

 
 

bug #11879: all mount-points lstat()-ed, but shouldn't be

Submitted by:  Tim Waugh <twaugh>
Submitted on:  Mon 07 Feb 2005 10:23:13 AM UTC  
 
Category: findSeverity: 3 - Normal
Item Group: Wrong resultStatus: Fixed
Privacy: PublicAssigned to: James Youngman <jay>
Originator Name: Open/Closed: Closed
Release: 4.2.15Fixed Release: 4.2.18

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue 01 Mar 2005 08:33:39 PM UTC, comment #9:

Note - this change caused bug #12181. That's fixed in CVS now, too.

James Youngman <jay>
Project AdministratorIn charge of this item.
Sun 20 Feb 2005 09:39:25 PM UTC, comment #8:

Yes, this is platform dependent. The Findutils manual explains everything. See bug #12044.

James Youngman <jay>
Project AdministratorIn charge of this item.
Sun 20 Feb 2005 12:38:06 PM UTC, comment #7:

Looks like this issue is now platform-dependent:
find version 4.2.18 no longer stats filesystems on GNU/Linux.

Dmitry V. Levin <ldv>
Sun 20 Feb 2005 07:20:28 AM UTC, comment #6:

(From Martin Buchholz <martin.buchholz@sun.com>)

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.

Anonymous
Sat 19 Feb 2005 12:56:46 PM UTC, comment #5:

The current release of findutils fixes this problem; please
download it from ftp://ftp.gnu.org/gnu/findutils or from one of
the mirror sites listed at http://www.gnu.org/software/findutils/.

James Youngman <jay>
Project AdministratorIn charge of this item.
Sun 13 Feb 2005 10:20:20 AM UTC, comment #4:

Dmitry, thanks for looking at this.

1. That hunk of test code existed only to place a marker in the strace output. It should have been removed earlier. It would have leaked a file descriptor!

2. You're right. That exposes find to the very security problem that this mechanism is supposed to fix! Stupid me. Fixed.

3. Most of the checks in dafely_chdir_lstat() just relate to checking and reporting problems with the lstat() results. I believe that these checks are no longer required, or do you believe I have missed something? I have moved complete_pending_execdirs() up into safely_chdir() though.

Thanks for the very useful feedback. I attach an updated patch. The updated patch is really a patch afainst findutils 4.2.17, so it also contains some minor changes to the find manpage which aren;t relevant here.

James Youngman <jay>
Project AdministratorIn charge of this item.
Sat 12 Feb 2005 11:50:30 PM UTC, comment #3:

1. this hunk looks odd:

+ time(NULL);
+ opendir(".");
+ time(NULL);
+

2. safely_chdir_nofollow() will switch to safely_chdir_lstat() if open() fails due to some race.

3. I'd prefer to avoid runtime fallback to safely_chdir_lstat() on platforms where fchdir() is known to work.

4. At this moment, safely_chdir_nofollow() does less then safely_chdir_lstat(). Maybe some stuff could be moved back to safely_chdir()?

Dmitry V. Levin <ldv>
Sat 12 Feb 2005 09:16:34 PM UTC, comment #2:

I have just checked the attached patch into CVS. This resolves the problem by using open(..., O_NOFOLLOW) followed by fchdir() instead of chdir(). This eliminates the need to lstat() the target directory before and after, hence eliminating the need for the mounted device list. However, fchdir() does not always work on all systems where the system call exists, and so in these circumstances we fall back on the previous implementation (enumerating the mounted devices at that point).

This is, to say the least, a significant change to the way that find works. The regression test suite passes, but then it only has 19 tests.

Please test this change thoroughly and let me know how it works for you. You can do this either by downloading the code from CVS (which I would recommend) or by applying the attached patch (against findutils-4.2.17). If I get negative feedback I can redo the fix or whatever, but if I get positive feedback we can move towards another release.

James Youngman <jay>
Project AdministratorIn charge of this item.
Mon 07 Feb 2005 01:53:01 PM UTC, comment #1:

This code was introduced to fix bug #9043 and bug #3998.

It would be nice if init_mounted_dev_list() was only called if we know we're going to need its result, but as I understand things by the time we know we need its (initial) result, it's too late to call it.

Any suggestions as to how all three bugs could be fixed simultaneously would be welcome. Thanks.

James Youngman <jay>
Project AdministratorIn charge of this item.
Mon 07 Feb 2005 10:23:13 AM UTC, original submission:

In main(), init_mounted_dev_list() is called unconditionally. This results in all mount-points getting lstat() called on them.

This happens when no options are given.

If one of the mount-points is a hard NFS mount, and the remote machine is down, find will never complete even though it should be able to without problems.

Tim Waugh <twaugh>

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #2544:  findutils-4.2.17-nofollow-try2.patch added by jay (24KiB - text/x-patch - Second proposed fix (as patch against 4.2.17 release))
file #2541:  findutils-4.2.17-nofollow.patch added by jay (20KiB - text/x-patch - First proposed fix for bug #11879 (patch against 4.2.17 release))

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

Do you think this task is very important?
If so, you can click here to add your encouragement to it.
This task has 0 encouragements so far.

Only logged-in users can vote.

 

Please enter the title of George Orwell's famous dystopian book (it's a date):

 

 

Follow 8 latest changes.

Date Changed By Updated Field Previous Value => Replaced By
Sat 19 Feb 2005 12:56:46 PM UTCjayOpen/ClosedOpen=>Closed
  Fixed ReleaseNone=>4.2.18
Sun 13 Feb 2005 10:20:20 AM UTCjayAttached File-=>Added findutils-4.2.17-nofollow-try2.patch, #2187
Sat 12 Feb 2005 11:50:30 PM UTCldvCarbon-Copy-=>Added ldv
Sat 12 Feb 2005 09:16:34 PM UTCjayStatusNone=>Fixed
  Attached File-=>Added findutils-4.2.17-nofollow.patch, #2184
Mon 07 Feb 2005 01:53:01 PM UTCjayItem GroupNone=>Wrong result
  Assigned toNone=>jay

Back to the top


Powered by Savane 3.1-cleanup1