[PATCH common] fix #8288: file: read first line: treat ESRCH like a missing file

[PATCH common] fix #8288: file: read first line: treat ESRCH like a missing file

[Lucas Barros]

Opening a file below /proc/<pid>/ can fail with ESRCH instead of ENOENT
when the process exits and gets reaped while the path is looked up, as
the permission check on the /proc/<pid> directory then no longer finds
the task. file_read_first_line() only treats ENOENT as "does not
exist" and dies otherwise.

This is hit by check_process_running(), which polls /proc/<pid>/stat
via read_proc_pid_stat(). On boot, startall uses it to wait for the
start workers of each startup order, outside of any eval, so a worker
exiting at the wrong moment aborts startall: pve-guests.service fails
and guests of later startup orders are not started at all.

A process that is gone is simply not running, so return undef for
ESRCH as well, just like for ENOENT. ESRCH is only returned by procfs
here, so other files are not affected.

Signed-off-by: Lucas Barros <lucasbarros.kam@gmail.com>
---
Reproduced on a PVE 9 node (kernel 7.0.14-28-pve, libpve-common-perl
9.2.8, 2 vCPUs) with a small script: one process forks short-lived
children and reaps them, another polls each child with
PVE::ProcFSTools::check_process_running(), like startall does with its
start workers. 60 seconds each:

before (installed libpve-common-perl):
  check_process_running polls: 414951, died: 58
    58x file '/proc/<pid>/stat' exists but open for reading failed -
        No such process

after (with this patch):
  check_process_running polls: 389918, died: 0

stopall and migrateall poll their workers with check_process_running()
as well.

In the same kind of loop, reading an already opened /proc/<pid>/stat
can also fail with ESRCH, but that already ends up as undef, as the
result of the read is not checked for errors.

Only file_read_first_line() is changed, as that is what the /proc
helpers use. file_read_last_line() has the same ENOENT check, I can
extend it there as well for consistency if preferred. An alternative
would be to handle this in read_proc_pid_stat() only, but the other
/proc/<pid>/ readers have the same race.

The test suite passes before and after ('make check', 347 vs. 349 ok),
the new test in file-test.pl fails without the change.
proxmox-perltidy reports no changes.

This patch was prepared with the help of an LLM based coding assistant;
I reviewed the change and ran the tests mentioned above myself.

 src/PVE/File.pm   |  4 +++-
 test/file-test.pl | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/PVE/File.pm b/src/PVE/File.pm
index 7d0f7ab..8162552 100644
--- a/src/PVE/File.pm
+++ b/src/PVE/File.pm
@@ -118,7 +118,9 @@ sub file_copy($filename, $dst, $max = undef, $perm = undef) {
 sub file_read_first_line($filename) {
     my $fh = IO::File->new($filename, "r");
     if (!$fh) {
-        return undef if $! == POSIX::ENOENT;
+        # opening a file below /proc/<pid>/ fails with ESRCH instead of ENOENT if the process
+        # exits during the path lookup, treat it the same as a file that does not exist
+        return undef if $! == POSIX::ENOENT || $! == POSIX::ESRCH;
         die "file '$filename' exists but open for reading failed - $!\n";
     }
     my $res = <$fh>;
diff --git a/test/file-test.pl b/test/file-test.pl
index 6c55105..ebba6c9 100755
--- a/test/file-test.pl
+++ b/test/file-test.pl
@@ -10,6 +10,8 @@ use PVE::File;
 
 use Encode;
 use File::Path qw(remove_tree);
+use POSIX ();
+use Test::MockModule;
 use Test::More;
 
 # TODO:
@@ -63,6 +65,20 @@ is_deeply(
     "simple write-read-compare test with wide-characters",
 );
 
+# a file below /proc/<pid>/ of a process that is just exiting can fail to open with ESRCH
+{
+    my $io_file = Test::MockModule->new('IO::File');
+    $io_file->redefine(
+        new => sub {
+            $! = POSIX::ESRCH;
+            return undef;
+        },
+    );
+    my $res = eval { PVE::File::file_read_first_line("/proc/1/stat") };
+    is($@, '', "ESRCH on open does not die");
+    is($res, undef, "ESRCH on open is treated like a file that does not exist");
+}
+
 done_testing();
 
 remove_tree($test_dir);
-- 
2.47.3