bugfindutils - Bugs: bug #27221, symlink_loop check broken by...

 
 

bug #27221: symlink_loop check broken by FTS_CWDFD

Submitted by:  Martin von Gagern <gagern>
Submitted on:  Mon 10 Aug 2009 06:42:37 PM UTC  
 
Category: findSeverity: 4 - Important
Item Group: Wrong resultStatus: Fixed
Privacy: PublicAssigned to: James Youngman <jay>
Originator Name: Open/Closed: Closed
Release: 4.5.4Fixed Release: 4.5.8

Add a New Comment(Rich Markup)
   

You are not logged in

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

 

Sun 11 Apr 2010 12:41:14 PM UTC, comment #4:

So, recent changes to the code have eliminated this particular problem.

However, there is still an architectural risk that code will use pathnames which are releative to some fd, without using the fd. For example, a function may call lstat() on a file name rather than fstatat.

At the moment nothing in the design of findutils mitigates this risk. The "good fix" I referred to earlier would be to modify the code such that pathnames and file descriptors to which they're relative should travel around the program as an undivided unit. That would be quite an extensive change.

However, this immmediate bug has been fixed, I think.

James Youngman <jay>
Project AdministratorIn charge of this item.
Sun 11 Apr 2010 12:27:18 PM UTC, comment #3:

Haven't dug through all the details again, but at least the example seems to work now: strace lists newfstatat invocations with a suitable directory handle for foo as first argument.

Martin von Gagern <gagern>
Thu 08 Apr 2010 10:19:27 AM UTC, comment #2:

Could you re-test this against findutils release 4.5.8 please?

James Youngman <jay>
Project AdministratorIn charge of this item.
Sat 20 Feb 2010 09:59:01 PM UTC, comment #1:

I'm working on a proper fix to this, but it turns out that a good fix will touch many many parts of the code, so it's taking me quite a long time to get it done.

James Youngman <jay>
Project AdministratorIn charge of this item.
Mon 10 Aug 2009 06:42:37 PM UTC, original submission:

In ftsfind.c there are two calls to a function symlink_loop. Both of these pass ent->fts_accpath as the argument, which in turn gets fed into stat resp. lstat.

It seems that fts_accpath only contains the base name of the file, not its full path. This behaviour is probably a consequence of the activation of the FTS_CWDFD flag in commit 214320ca.

Either fts_path should be passed instead of fts_accpath, or fstatat would have to be used together with fts_cwd_fd.

I saw traces of this issue in an strace output while investigating bug 27213. So steps to reproduce this are:
$ mkdir -p foo/bar
$ chmod a-x foo
$ strace find foo
...
fstatat64(6, "bar", 0x9d72100, AT_SYMLINK_NOFOLLOW)
= -1 EACCES (Permission denied)
dup(6) = 5
fcntl64(5, F_GETFD) = 0
fcntl64(5, F_SETFD, FD_CLOEXEC) = 0
lstat64("bar", 0xbfb8f104)
= -1 ENOENT (No such file or directory)

The -L command line switch, which enables FTS_LOGICAL, will cause FTS_NOCHDIR to be enabled, in which case fts_accpath is equal to fts_path, and things work out fine. Therefore the example from bug #19605 can't be used to demonstrate this issue here.

Assuming that the directory containing a node is valid, I cannot imagine a node within where lstat could result in an ELOOP error. And invalid directories would have to be search roots, which get a somewhat different treatment not involving symlink_loop. In other words, I belive an unfollowed symlink can never be a part of a symlink loop. Therefore the lstat part of symlink_loop seems pointless.

Furthermore, I cannot imagine how a node could result in FTS_SLNONE and be part of a loop at the same time. So the check there doesn't make sense to me.

As for the FTS_NS case: in this case I'd like to hear about loop errors, as well as any other kind of error causing the stat information to be unavailable. I can get the error of the stat from the fts_errno variable, so I don't need to stat again. Doing this fixes the part of bug #27213 where I complain about the absence of an error message for nodes in non-executable dirs.

So I suggest:
1. Always issue an error message for NTF_NS
2. Never issue an error message for FTS_SLNONE
3. Drop the symlink_loop function, as it serves no purpose

Martin von Gagern <gagern>

 

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

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jay (Posted a comment)
  • -unavailable- added by gagern (Submitted the item)
  •  

    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 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 11 Apr 2010 12:41:14 PM UTCjayStatusNeed Info=>Fixed
      Open/ClosedOpen=>Closed
      Fixed ReleaseNone=>4.5.8
    Sun 11 Apr 2010 11:52:00 AM UTCjayStatusNone=>Need Info
    Sat 20 Feb 2010 09:59:01 PM UTCjaySeverity3 - Normal=>4 - Important
      Assigned toNone=>jay

    Back to the top


    Powered by Savane 3.1-cleanup1