bugfindutils - Bugs: bug #27213, consider_visiting: invalid...

 
 

bug #27213: consider_visiting: invalid assertion for FTS_NS due to access permissions.

Submitter:  Martin von Gagern <gagern>
Submitted:  Mon 10 Aug 2009 10:08:34 AM UTC
Votes: 11
 
Category:  find Severity:  3 - Normal
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
   

Jump to the original submission

Wed 07 Apr 2010 11:11:13 PM UTC, comment #8: 

Fixed in 4.5.8, which I just released.

James Youngman <jay>
Group administrator
Tue 06 Apr 2010 10:37:04 PM UTC, comment #7: 

I've just pushed the fix.  It will be included in the next 4.3.x release.

James Youngman <jay>
Group administrator
Wed 03 Feb 2010 04:25:43 PM UTC, comment #6: 

Thanks, Eric. It's good to know that someone has taken notice of this, and that eventually someone will have a closer look. Seeing that this report is almost six months old, a few more weeks don't sound too bad. Looking forward to the review.

Martin von Gagern <gagern>
Wed 03 Feb 2010 01:35:49 PM UTC, comment #5: 

The problem is not lack of interest, but lack of time.  I've queued this on my list of things to get to, if no one beats me, but I am probably 3 or more weeks out from getting to it.

Eric Blake <ericb>
Group administrator
Wed 03 Feb 2010 08:45:03 AM UTC, comment #4: 

Still no reply, so I sent another mail, this time to bug-findutils:
http://thread.gmane.org/gmane.comp.gnu.findutils.bugs/4304
http://lists.gnu.org/archive/html/bug-findutils/2010-01/msg00012.html
No answer there yet either, but I keep hoping.

Martin von Gagern <gagern>
Mon 23 Nov 2009 08:40:23 PM UTC, comment #3: 

As this report here didn't seem to receive any attention, I sent my patch to the findutils-patches mailing list:
http://thread.gmane.org/gmane.comp.gnu.findutils.patches/320

Martin von Gagern <gagern>
Wed 26 Aug 2009 09:46:15 AM UTC, comment #2: 

I attached a patch that I believe should fix the issue. At least now "find foo" does work, as does "find foo -ls".

With "works" I mean that it does print an error message and continue traversal, instead of aborting due to a failed assertion.

Changes:
1. Always print error message for FTS_NS. I can't imagine a case where you wouldn't want to learn about a stat error, especially if this could prevent you from knowing whether an inode is a directory or not.
2. assert stat.type == 0 in the case of FTS_NS. This is closer to the old !state.have_type, so I guess that's what was meant.
3. Moved some post-condition assertions in get_info from the failure case to the success case.
4. Added some comments, specially pointing out an issue with duplicate error messages.

Those duplicate error messages should be avoided one day, but I'd rather have duplicate error messages than failed assertions, so I don't think that the introduction of these should block the application of this patch here.

(file #18644)

Martin von Gagern <gagern>
Mon 10 Aug 2009 04:41:05 PM UTC, comment #1: 

Two corrections first:

> I find that an assertion "state.have_type" had been changed
> into "state.type != 0" which sounds like the exact opposite
> of the original assumption.


The former had been "!state.have_type".

> FTS_NSOK (indicating an omitted stat due to ),


... due to it not being requested in the first place.


I also had a look at behaviour in acb82fe4^ = cd123070:
$ find foo
foo
find: `foo': Permission denied

The (in my opinion better) error message comes from the FTS_ERR handler in consider_visiting. This error message doesn't even change with acb82fe4 itself, but only later in 214320ca when FTS_CWDFD got activated. Makes sense, as the old implementation would have chdir'ed into a non-existing directory.

There is an error handling block for ent->fts_info == FTS_NS, but it doesn't emit any message if ent->fts_level != 0 and no symlink loop is deteced. Adding yet another error message should be easy, and I can't see why that isn't done already.

BTW: symlink_loop got broken by FTS_CWDFD as well, will report a separate bug for this.

Notice that bug #25294 comment #0 by Eric Blake probably holds for this issue here as well: there seems to be no spec saying anything about ent->fts_statp in the FTS_NS and FTS_NSOK case. I assume that the gnulib implementation has some concept here, but it should be documented before other projects rely on it, I think.

Martin von Gagern <gagern>
Mon 10 Aug 2009 10:08:34 AM UTC, original submission:  

This bug is related to bug #25294 but doesn't match its description. It is also covered by http://bugs.gentoo.org/253570 .

Steps to reproduce:
$ mkdir -p foo/bar
$ chmod a-x foo
$ find foo
foo
find: ftsfind.c:475: consider_visiting: Assertion `ent->fts_info == 11 ||
state.type != 0' failed.
Aborted

Could reproduce with current git master as well.
I assume the problem here lies with commit acb82fe4 and hasn't been fixed by commit e3bcac43.

Looking at the former, I find that an assertion "state.have_type" had been changed into "state.type != 0" which sounds like the exact opposite of the original assumption. As all of this is in the case of either FTS_NS (indicating a failed stat) or FTS_NSOK (indicating an omitted stat due to ), I would assume that state.type == 0 should be expected at least in the FTS_NS case.

So maybe you want to have these assertions instead:
assert (ent->fts_info != FTS_NS || state.type == 0);
assert (ent->fts_info != FTS_NSOK || state.type != 0);
These can be read as logical implications:
"Assert that if ent->fts_info == FTS_NS, then state.type == 0"
and
"Assert that if ent->fts_info == FTS_NSOK, then state.type != 0"

I'm none too sure about the FTS_NSOK case. In discussing this, it might make sense to investigate both why these assertions should be expected to be true from an abstract point of view as well as how they affect correct behaviour.

I'm also wondering whether some kind of error message for an un-stat-able node would be better than silently using wrong file modes and perhaps even pruning the tree, as the implementation with the above fixes would do instead.

Martin von Gagern <gagern>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #18644:  0001-Bug-27213-avoid-failed-assertions-for-non-executable.patch added by gagern (4KiB - application/octet-stream - Patch version 1)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by pterjan (Voted in favor of this item)
  • -email is unavailable- added by jay (Updated the item)
  • -email is unavailable- added by pipping
  • -email is unavailable- added by ericb (Posted a comment)
  • -email is unavailable- added by gagern (Voted in favor of this item)
  • -email is unavailable- added by gagern (Submitted the item)
  •  

    There are 11 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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-04-07 jay Open/ClosedOpen Closed
        Fixed ReleaseNone 4.5.8
    2010-04-06 jay StatusCode Review Fixed
    2010-04-05 pterjan Carbon-Copy- Added pterjan
    2010-04-04 jay StatusNone Code Review
        Assigned toNone jay
    2010-03-31 pipping Carbon-Copy- Added pipping
    2009-08-26 gagern Carbon-Copy- Added gagern
        Attached File- Added 0001-Bug-27213-avoid-failed-assertions-for-non-executable.patch, #18644

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code