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

 
 

bug #27221: symlink_loop check broken by FTS_CWDFD

Submitter:  Martin von Gagern <gagern>
Submitted:  Mon 10 Aug 2009 06:42:37 PM UTC
   
 
Category:  find Severity:  4 - Important
Item Group:  Wrong result Status:  Fixed
Privacy:  Public Assigned to:  jay
Originator Name:  Open/Closed:  Closed
Release:  4.5.4 Fixed Release:  4.5.8
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group administrator
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>
Group administrator
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>
Group administrator
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 Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

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

    Date Changed by Updated Field Previous Value => Replaced by
    2010-04-11 jay StatusNeed Info Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 4.5.8
    2010-04-11 jay StatusNone Need Info
    2010-02-20 jay Severity3 - Normal 4 - Important
        Assigned toNone jay

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code