[PATCH common 1/3] tools: lock file: add helper to close inherited lock handles

["Michael Köppl" <m.koeppl@proxmox.com>]

A child forked while its parent holds a lock from lock_file_full()
inherits the locked file handle. The lock then is held until the child
exits too, even after the parent released it.

Keep the locked handle instead of a flag in the scalar that
$lock_handles weakly references. Add a helper that lets a child close
its copies of the handles locked by its ancestors. The ancestor keeps
holding the lock, since its own handle stays open.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/Tools.pm  | 23 +++++++++++++++-----
 test/lock_file.pl | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 9176b358..2f2123c1 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -198,7 +198,8 @@ sub run_with_timeout {
 # Since this is the only way we lock now and we don't have the old
 # 'lock(); code(); unlock();' pattern anymore we do not actually need to
 # count how deep we're nesting. Therefore this hash now stores a weak reference
-# to a boolean telling us whether we already have a lock.
+# to a scalar holding the locked file handle. If the lock has not been acquired
+# yet, it holds a false value.
 
 my $lock_handles = {};
 
@@ -243,7 +244,7 @@ sub lock_file_full {
     if (!$checkptr || !$$checkptr) {
         # We cannot create a weak reference in a single atomic step, so we first
         # create a false-value, then create a reference to it, then weaken it,
-        # and after successfully locking the file we change the boolean value.
+        # and after successfully locking the file we store the handle in it.
         #
         # The reason for this is that if an outer SIGALRM throws an exception
         # between creating the reference and weakening it, a subsequent call to
@@ -253,8 +254,8 @@ sub lock_file_full {
         #
         # An externally triggered exception elsewhere in the code will cause the
         # weak reference to become 'undef', and since the file handle is only
-        # stored in the local scope in $local_fh, the file will be closed by
-        # perl's cleanup routines as well.
+        # stored in the local scope in $local_fh and $check, the file will be
+        # closed by perl's cleanup routines as well.
         #
         # This still assumes that an IO::File handle can properly deal with such
         # exceptions thrown during its own destruction, but that's up to perls
@@ -266,7 +267,7 @@ sub lock_file_full {
             $@ = "can't lock file '$filename' - $@";
             return undef;
         }
-        $check = 1;
+        $check = $local_fh;
     }
     $res = eval { &$code(@param); };
     return undef if $@;
@@ -279,6 +280,18 @@ sub lock_file {
     return lock_file_full($filename, $timeout, 0, $code, @param);
 }
 
+# Close this process's copies of lock handles inherited from its ancestors, so
+# it does not keep their locks held after they released them.
+sub close_inherited_lock_handles {
+    for my $pid (keys %$lock_handles) {
+        next if $pid == $$;
+        for my $checkptr (values $lock_handles->{$pid}->%*) {
+            close($$checkptr) if $checkptr && $$checkptr;
+        }
+        delete $lock_handles->{$pid};
+    }
+}
+
 # file_ helper for backward compat, prefer PVE::File directly for new usage.
 # TODO: remove with PVE 10 / Debian Forky releases.
 sub file_set_contents {
diff --git a/test/lock_file.pl b/test/lock_file.pl
index 83965c55..ffa2404a 100755
--- a/test/lock_file.pl
+++ b/test/lock_file.pl
@@ -5,6 +5,7 @@ use strict;
 use warnings;
 
 use Socket;
+use Fcntl qw(:flock);
 use POSIX (); # don't import assert()
 
 use PVE::Tools 'lock_file_full';
@@ -91,6 +92,54 @@ sub forktest1($) {
     close($fmain);
 }
 
+sub lock_is_free {
+    open(my $fh, '>>', $name) or die "open(): $!\n";
+    return flock($fh, LOCK_EX | LOCK_NB);
+}
+
+sub inherited_lock_test {
+    my ($close_in_child) = @_;
+
+    socketpair(my $fparent, my $fchild, AF_UNIX, SOCK_STREAM, PF_UNSPEC)
+        or die "socketpair(): $!\n";
+
+    my $pid;
+    lock_file_full(
+        $name,
+        10,
+        0,
+        sub {
+            $pid = fork() // die "fork(): $!\n";
+            if (!$pid) {
+                close($fparent);
+                PVE::Tools::close_inherited_lock_handles() if $close_in_child;
+                print {$fchild} "1\n";
+                $fchild->flush();
+                <$fchild>; # wait for parent to finish checks
+                POSIX::_exit(0);
+            }
+            close($fchild);
+            my $line = <$fparent>;
+            die "child failed to start\n" if !$line || $line ne "1\n";
+            die "lock released while the parent still holds it\n" if lock_is_free();
+        },
+    );
+
+    my $err = $@;
+    my $free = !$err && lock_is_free();
+    print {$fparent} "2\n";
+    $fparent->flush();
+    waitpid($pid, 0) if $pid;
+    close($fparent);
+    die $err if $err;
+
+    if ($close_in_child) {
+        die "child kept the inherited lock held\n" if !$free;
+    } else {
+        die "child did not inherit the lock\n" if $free;
+    }
+}
+
 eval {
     # Regular lock:
     new();
@@ -143,6 +192,11 @@ eval {
     # Can we get two simultaneous shared locks? (shared=1)
     forktest1(0);
     forktest1(1);
+
+    # Lock held by child after parent released it?
+    inherited_lock_test(0);
+    # Lock free once child closed inherited copies?
+    inherited_lock_test(1);
 };
 my $err = $@;
 system("rm $name*");
-- 
2.47.3