On 12/22/21 09:34, Andrea Monaco wrote:
> Hello,
> 
> when searching by inode number (-inum predicate) on my GNU/Hurd system,
> find crashes due to assertion "p->st_ino" in get_info failing.
> 
> The same assertion is also done in pred.c at line 501.  Inode numbers
> are often assumed to start from one, but apparently this is not mandated
> by POSIX and is not true on my system.
> 
> Do that assertions protect against some type of errors?  Otherwise we
> could remove them.
> 
> Thanks,
> Andrea Monaco

Thanks for the bug report.
I managed to reproduce in a local GNU/Hurd VM on VirtualBox, see:
  https://www.researchut.com/blog/debian-hurd-on-virtualbox/

Indeed, GNU/Hurd is using inode number Zero e.g. for /dev/console and /dev/tty.

The first patch fixes a side effect of '-D stat' on SELinux handling which
I found while reading adjacent code; pushed at:
  https://git.sv.gnu.org/cgit/findutils.git/commit/?id=68979e578

The 2nd patch fixes the reported issue; pushed at:
  https://git.sv.gnu.org/cgit/findutils.git/commit/?id=9290525c7

Have a nice day,
Berny
From 68979e578cbea71211af45685561df52232623d1 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <m...@bernhard-voelker.de>
Date: Sat, 25 Dec 2021 16:14:59 +0100
Subject: [PATCH 1/2] find: avoid '-D stat' side effect on SELinux handling
 with -L or -H

With the '-D stat' debugging option turned on, 'find -L' determined the
SELinux context information as if the -L (or -H) option was not given:

  $ strace -ve getxattr,lgetxattr find -L . -maxdepth 0 -printf '%Z:%p\n'
  getxattr(".", "security.selinux", 0x55b29a2b2d40, 255) = -1 ENODATA (No data available)
  ...

  $ strace -ve getxattr,lgetxattr find -D stat -L . -maxdepth 0 -printf '%Z:%p\n'
  lgetxattr(".", "security.selinux", 0x5649c91d8d40, 255) = -1 ENODATA (No data available)
  ...

* find/parser.c (set_follow_state): Move the DebugStat handling after
the switch statement, thus eliminating the if/else.

Bug present since adding the SELinux implementation in v4.5.5-42-g1a05af6a.
---
 NEWS          |  6 ++++++
 find/parser.c | 49 ++++++++++++++++++++++++-------------------------
 2 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/NEWS b/NEWS
index 02c1c55d..046eaddb 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,12 @@ GNU findutils NEWS - User visible changes.      -*- outline -*- (allout)
   update now skips (fuse-mounted) s3fs filesystems by default,
   i.e., unless PRUNEFS is set.
 
+** Bug Fixes
+
+  'find -D stat -L ...' no longer determines SELinux security information as
+  if the -L option was not given.
+  [Bug present since the SELinux implementation in 4.5.6]
+
 ** Documentation Changes
 
   The find.1 man page and the Texinfo manual now show environment variables
diff --git a/find/parser.c b/find/parser.c
index f5d100a2..454764f3 100644
--- a/find/parser.c
+++ b/find/parser.c
@@ -503,6 +503,30 @@ get_stat_Ytime (const struct stat *p,
 void
 set_follow_state (enum SymlinkOption opt)
 {
+  switch (opt)
+    {
+    case SYMLINK_ALWAYS_DEREF:  /* -L */
+      options.xstat = optionl_stat;
+      options.x_getfilecon = optionl_getfilecon;
+      options.no_leaf_check = true;
+      break;
+
+    case SYMLINK_NEVER_DEREF:	/* -P (default) */
+      options.xstat = optionp_stat;
+      options.x_getfilecon = optionp_getfilecon;
+      /* Can't turn no_leaf_check off because the user might have specified
+       * -noleaf anyway
+       */
+      break;
+
+    case SYMLINK_DEREF_ARGSONLY: /* -H */
+      options.xstat = optionh_stat;
+      options.x_getfilecon = optionh_getfilecon;
+      options.no_leaf_check = true;
+    }
+
+  options.symlink_handling = opt;
+
   if (options.debug_options & DebugStat)
     {
       /* For DebugStat, the choice is made at runtime within debug_stat()
@@ -510,31 +534,6 @@ set_follow_state (enum SymlinkOption opt)
        */
       options.xstat = debug_stat;
     }
-  else
-    {
-      switch (opt)
-	{
-	case SYMLINK_ALWAYS_DEREF:  /* -L */
-	  options.xstat = optionl_stat;
-	  options.x_getfilecon = optionl_getfilecon;
-	  options.no_leaf_check = true;
-	  break;
-
-	case SYMLINK_NEVER_DEREF:	/* -P (default) */
-	  options.xstat = optionp_stat;
-	  options.x_getfilecon = optionp_getfilecon;
-	  /* Can't turn no_leaf_check off because the user might have specified
-	   * -noleaf anyway
-	   */
-	  break;
-
-	case SYMLINK_DEREF_ARGSONLY: /* -H */
-	  options.xstat = optionh_stat;
-	  options.x_getfilecon = optionh_getfilecon;
-	  options.no_leaf_check = true;
-	}
-    }
-  options.symlink_handling = opt;
 }
 
 
-- 
2.34.1

From 9290525c774290e451b13f7cdf81262d7656e3ed Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <m...@bernhard-voelker.de>
Date: Sun, 26 Dec 2021 11:09:22 +0100
Subject: [PATCH 2/2] find: fix visiting of files with inode number Zero

On GNU/Hurd, the value 0 is a valid inode number, and is e.g. used
for /dev/console and /dev/tty.  The find(1) program aborted on this
platform when the user specified the -inum test and when the search
visited such a file.

  $ find /dev/null /dev/tty -inum 40799 -printf '%i:%p\n'
  40799:/dev/null
  find: util.c:330: get_info: Assertion `p->st_ino' failed.
  Aborted

Likewise, 'find -printf %i' aborted when hitting such a file.

* find/defs.h (get_info): Remove declaration.
* find/pred.c (pred_inum): Remove the redundant assert for ST_INO
as parse_inum sets need_inum=true which ensures that the inode number
is known.
* find/util.c (get_info): Declare static, and simplify: remove the
assertions for the inode number and file type.
While at it, add condition !state.have_stat in the need_stat case
for consistency.
* tests/find/inode-zero.sh: Add test.
* tests/local.mk (all_tests): Reference it.

Problem introduced by the inum optimisation in commit 2bf001636e6.

Reported by Andrea Monaco <andrea.mon...@autistici.org> in
https://lists.gnu.org/r/bug-findutils/2021-12/msg00008.html
---
 NEWS                     |  5 ++++
 find/defs.h              |  1 -
 find/pred.c              |  2 --
 find/util.c              | 31 ++++-------------------
 tests/find/inode-zero.sh | 54 ++++++++++++++++++++++++++++++++++++++++
 tests/local.mk           |  1 +
 6 files changed, 65 insertions(+), 29 deletions(-)
 create mode 100755 tests/find/inode-zero.sh

diff --git a/NEWS b/NEWS
index 046eaddb..ffde4720 100644
--- a/NEWS
+++ b/NEWS
@@ -20,6 +20,11 @@ GNU findutils NEWS - User visible changes.      -*- outline -*- (allout)
   if the -L option was not given.
   [Bug present since the SELinux implementation in 4.5.6]
 
+  'find -inum' and 'find -printf %i' now also work on platforms which allow
+  the inode number Zero; e.g. the GNU/Hurd uses inode number 0 for /dev/console.
+  Previously, find(1) would abort when visiting such a file.
+  [Bug present since FINDUTILS_4_5_4-1.]
+
 ** Documentation Changes
 
   The find.1 man page and the Texinfo manual now show environment variables
diff --git a/find/defs.h b/find/defs.h
index cb519136..63ab7eda 100644
--- a/find/defs.h
+++ b/find/defs.h
@@ -519,7 +519,6 @@ bool apply_predicate(const char *pathname, struct stat *stat_buf, struct predica
 
 
 /* util.c. */
-int get_info (const char *pathname, struct stat *p, struct predicate *pred_ptr);
 bool following_links (void);
 bool digest_mode (mode_t *mode, const char *pathname, const char *name, struct stat *pstat, bool leaf);
 bool default_prints (struct predicate *pred);
diff --git a/find/pred.c b/find/pred.c
index bf995723..338ac14a 100644
--- a/find/pred.c
+++ b/find/pred.c
@@ -498,8 +498,6 @@ pred_inum (const char *pathname, struct stat *stat_buf, struct predicate *pred_p
 {
   (void) pathname;
 
-  assert (stat_buf->st_ino != 0);
-
   switch (pred_ptr->args.numinfo.kind)
     {
     case COMP_GT:
diff --git a/find/util.c b/find/util.c
index afd9880e..59f80659 100644
--- a/find/util.c
+++ b/find/util.c
@@ -280,7 +280,7 @@ get_statinfo (const char *pathname, const char *name, struct stat *p)
 /* Get the stat/type/inode information for a file, if it is not
  * already known.   Returns 0 on success (or if we did nothing).
  */
-int
+static int
 get_info (const char *pathname,
 	  struct stat *p,
 	  struct predicate *pred_ptr)
@@ -290,7 +290,7 @@ get_info (const char *pathname,
   /* If we need the full stat info, or we need the type info but don't
    * already have it, stat the file now.
    */
-  if (pred_ptr->need_stat)
+  if (pred_ptr->need_stat && !state.have_stat)
     {
       todo = true;		/* need full stat info */
     }
@@ -316,31 +316,10 @@ get_info (const char *pathname,
     }
   if (todo)
     {
-      int result = get_statinfo (pathname, state.rel_pathname, p);
-      if (result != 0)
-	{
-	  return -1;		/* failure. */
-	}
-      else
-	{
-	  /* Verify some postconditions.  We can't check st_mode for
-	     non-zero-ness because of Savannah bug #16378 (which is
-	     that broken NFS servers can return st_mode==0). */
-	  if (pred_ptr->need_type)
-	    {
-	      assert (state.have_type);
-	    }
-	  if (pred_ptr->need_inum)
-	    {
-	      assert (p->st_ino);
-	    }
-	  return 0;		/* success. */
-	}
-    }
-  else
-    {
-      return 0;			/* success; nothing to do. */
+      if (get_statinfo (pathname, state.rel_pathname, p) != 0)
+	return -1;		/* failure. */
     }
+  return 0;			/* success, or nothing to do. */
 }
 
 /* Determine if we can use O_NOFOLLOW.
diff --git a/tests/find/inode-zero.sh b/tests/find/inode-zero.sh
new file mode 100755
index 00000000..7bfc2698
--- /dev/null
+++ b/tests/find/inode-zero.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+# Ensure find(1) treats inode number 0 correctly.
+
+# Copyright (C) 2021 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; fu_path_prepend_
+print_ver_ find
+
+# Skip test unless we find a file with inode number 0.
+# GNU/Hurd uses inode 0 for /dev/console.
+f='/dev/console'
+test -e "${f}" \
+  && ino=$( stat -c '%i' "${f}" ) \
+  && test "${ino}" = '0' \
+  || skip_ "no file with inode number 0 here"
+
+echo "${f}" > exp || framework_failure_
+
+# Ensure -inum works.
+# Find by exact inode number 0.
+find "${f}" -inum 0 >out 2>err || fail=1
+compare exp out || fail=1
+compare /dev/null err || fail=1
+
+# Find by inode number <1.
+find "${f}" -inum -1 >out 2>err || fail=1
+compare exp out || fail=1
+compare /dev/null err || fail=1
+
+# No match with unrelated inode number.
+find "${f}" -inum 12345 >out 2>err || fail=1
+compare /dev/null out || fail=1
+compare /dev/null err || fail=1
+
+# Ensure '-printf "%i"' works.
+echo 0 > exp || framework_failure_
+find "${f}" -printf '%i\n' >out 2>err || fail=1
+compare exp out || fail=1
+compare /dev/null err || fail=1
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index 17efc591..5ddcaaf9 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -107,6 +107,7 @@ check-root:
 all_tests = \
   tests/misc/help-version.sh \
   tests/find/depth-unreadable-dir.sh \
+  tests/find/inode-zero.sh \
   tests/find/many-dir-entries-vs-OOM.sh \
   tests/find/name-lbracket-literal.sh \
   tests/find/printf_escapechars.sh \
-- 
2.34.1

Reply via email to