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

[Tony Abou-Assaleh-4]

URL:
  <http://savannah.gnu.org/bugs/?27213>

                 Summary: consider_visiting: invalid assertion for FTS_NS due
to access permissions.
                 Project: findutils
            Submitted by: gagern
            Submitted on: Mo 10 Aug 2009 10:08:34 GMT
                Category: find
                Severity: 3 - Normal
              Item Group: Wrong result
                  Status: None
                 Privacy: Public
             Assigned to: None
         Originator Name:
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any
                 Release: 4.5.4
           Fixed Release: None

    _______________________________________________________

Details:

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.




    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27213>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/

[Tony Abou-Assaleh-4]

Follow-up Comment #1, bug #27213 (project findutils):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27213>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/

[Tony Abou-Assaleh-4]

Follow-up Comment #2, bug #27213 (project findutils):

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)
    _______________________________________________________

Additional Item Attachment:

File name: 0001-Bug-27213-avoid-failed-assertions-for-non-executable.patch
Size:3 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27213>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/

[Tony Abou-Assaleh-4]

Follow-up Comment #3, bug #27213 (project findutils):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?27213>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/