From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id B85E61FF0DF for ; Thu, 05 Nov 2026 17:37:02 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 8D37F2167B; Thu, 05 Nov 2026 17:36:44 +0100 (CET) From: =?UTF-8?q?Michael=20K=C3=B6ppl?= To: pve-devel@lists.proxmox.com Subject: [PATCH common 1/3] tools: lock file: add helper to close inherited lock handles Date: Thu, 5 Nov 2026 17:36:35 +0100 Message-ID: <20261105163637.1318072-2-m.koeppl@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261105163637.1318072-1-m.koeppl@proxmox.com> References: <20261105163637.1318072-1-m.koeppl@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793896598452 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.210 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: O2BT7BTQUY5PRB6WVRESJWQJGLSRF2YO X-Message-ID-Hash: O2BT7BTQUY5PRB6WVRESJWQJGLSRF2YO X-MailFrom: m.koeppl@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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