Re: find crashes when searching by inode number (-inum) on GNU/Hurd

Bernhard Voelker <[email protected]>
Newsgroups gmane.comp.gnu.findutils.bugs
Message-ID <[email protected]>
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
0001-find-avoid-D-stat-side-effect-on-SELinux-handling-wi.patch (text/x-patch, 3.4 KB)
From 68979e578cbea71211af45685561df52232623d1 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <[email protected]>
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
0002-find-fix-visiting-of-files-with-inode-number-Zero.patch (text/x-patch, 6.9 KB)
From 9290525c774290e451b13f7cdf81262d7656e3ed Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <[email protected]>
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 <[email protected]> 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.