From 247cff335f6019ea1aa4c72fab85f7d552558c06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 22 Oct 2014 03:53:30 +0200 Subject: [PATCH] core: fix race condition in qubes.xml locking (#906) QubesVmCollection.save() overrides qubes.xml by creating new file, then renaming it over the old one. If any process has that (old) file open at the same time - especially while waiting on lock_db_for_writing() - it will end up in accessing old, already unlinked file. The exact calls would look like: P1 P2 lock_db_for_writing fd = open('qubes.xml') fcntl(fd, F_SETLK, ...) lock_db_for_writing fd = open('qubes.xml') fcntl(fd, F_SETLK, ...) ... save(): open(temp-file) write(temp-file, ...) ... flush(temp-file) rename(temp-file, 'qubes.xml') close(fd) // close old file lock_db_for_writing succeed *** fd points at already unlinked file unlock_db close(qubes.xml) To fix that problem, added a check if (already locked) file is still the same as qubes.xml. --- core/qubes.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/core/qubes.py b/core/qubes.py index ea630d1e..89259995 100755 --- a/core/qubes.py +++ b/core/qubes.py @@ -526,12 +526,28 @@ class QubesVmCollection(dict): self.save() def lock_db_for_reading(self): - self.qubes_store_file = open (self.qubes_store_filename, 'r') - fcntl.lockf (self.qubes_store_file, fcntl.LOCK_SH) + # save() would rename the file over qubes.xml, _then_ release lock, + # so we need to ensure that the file for which we've got the lock is + # still the right file + while True: + self.qubes_store_file = open (self.qubes_store_filename, 'r') + fcntl.lockf(self.qubes_store_file, fcntl.LOCK_SH) + if os.fstat(self.qubes_store_file.fileno()) == os.stat( + self.qubes_store_filename): + break + self.qubes_store_file.close() def lock_db_for_writing(self): - self.qubes_store_file = open (self.qubes_store_filename, 'r+') - fcntl.lockf (self.qubes_store_file, fcntl.LOCK_EX) + # save() would rename the file over qubes.xml, _then_ release lock, + # so we need to ensure that the file for which we've got the lock is + # still the right file + while True: + self.qubes_store_file = open (self.qubes_store_filename, 'r+') + fcntl.lockf(self.qubes_store_file, fcntl.LOCK_EX) + if os.fstat(self.qubes_store_file.fileno()) == os.stat( + self.qubes_store_filename): + break + self.qubes_store_file.close() def unlock_db(self): # intentionally do not call explicit unlock to not unlock the file