diff --git a/msal_extensions/cache_lock.py b/msal_extensions/cache_lock.py index e5ddf2b..4c9c096 100644 --- a/msal_extensions/cache_lock.py +++ b/msal_extensions/cache_lock.py @@ -1,8 +1,6 @@ """Provides a mechanism for not competing with other processes interacting with an MSAL cache.""" import os import sys -import errno -import time import logging import portalocker # pylint: disable=import-error @@ -18,57 +16,39 @@ class CrossPlatLock(object): """Offers a mechanism for waiting until another process is finished interacting with a shared resource. This is specifically written to interact with a class of the same name in the .NET extensions library. + + Fix: Use blocking LOCK_EX (no LOCK_NB) and never delete the lockfile. + The original design had two races: + 1. LOCK_NB caused 300+ concurrent processes to bypass the lock (LockError raised instead of + waiting), allowing concurrent writes to the shared .tmp file. + 2. Deleting the lockfile after releasing let another process create a new inode at the same + path, so two processes could each hold "exclusive" locks on different inodes simultaneously. + Both races caused the shared .tmp file to receive concurrent interleaved writes, producing + JSONDecodeError("Extra data") in the cache. """ def __init__(self, lockfile_path): self._lockpath = lockfile_path self._lock = portalocker.Lock( lockfile_path, - mode='wb+', - # In posix systems, we HAVE to use LOCK_EX(exclusive lock) bitwise ORed - # with LOCK_NB(non-blocking) to avoid blocking on lock acquisition. - # More information here: - # https://docs.python.org/3/library/fcntl.html#fcntl.lockf - flags=portalocker.LOCK_EX | portalocker.LOCK_NB, - # Support for passing through arguments to the open syscall - # was added in Portalocker v1.4.0 (2019-02-11). - buffering=0, + # Use append mode: opens without truncating, creates file if absent. + # Never truncate the lockfile — that would corrupt another holder's fd. + mode='a', + # Use blocking LOCK_EX only (no LOCK_NB). + # With LOCK_NB, all but one of 300 concurrent processes would raise LockError + # immediately instead of waiting, bypassing the critical section entirely. + flags=portalocker.LOCK_EX, ) - def _try_to_create_lock_file(self): - timeout = 5 - check_interval = 0.25 - current_time = getattr(time, "monotonic", time.time) - timeout_end = current_time() + timeout - pid = os.getpid() - while timeout_end > current_time(): - try: - with open(self._lockpath, 'x'): # pylint: disable=unspecified-encoding - return True - except ValueError: # This needs to be the first clause, for Python 2 to hit it - logger.warning("Python 2 does not support atomic creation of file") - return False - except FileExistsError: # Only Python 3 will reach this clause - logger.debug( - "Process %d found existing lock file, will retry after %f second", - pid, check_interval) - time.sleep(check_interval) - return False - def __enter__(self): pid = os.getpid() - if not self._try_to_create_lock_file(): - logger.warning("Process %d failed to create lock file", pid) file_handle = self._lock.__enter__() - file_handle.write('{} {}'.format(pid, sys.argv[0]).encode('utf-8')) # pylint: disable=consider-using-f-string + logger.debug("Process %d acquired token cache lock", pid) return file_handle def __exit__(self, *args): + # Release the lock but leave the lockfile on disk. + # Deleting + recreating the file changes its inode, which breaks the POSIX guarantee + # that fcntl locks are per-(process, inode) pair — two processes can then each hold + # an "exclusive" lock on different inodes of the same path simultaneously. self._lock.__exit__(*args) - try: - # Attempt to delete the lockfile. In either of the failure cases enumerated below, it is - # likely that another process has raced this one and ended up clearing or locking the - # file for itself. - os.remove(self._lockpath) - except OSError as ex: # pylint: disable=invalid-name - if ex.errno not in (errno.ENOENT, errno.EACCES): - raise + logger.debug("Process %d released token cache lock", os.getpid()) diff --git a/msal_extensions/persistence.py b/msal_extensions/persistence.py index 8056c30..63e10e5 100644 --- a/msal_extensions/persistence.py +++ b/msal_extensions/persistence.py @@ -150,8 +150,20 @@ def __init__(self, location): def save(self, content): # type: (str) -> None """Save the content into this persistence""" - with os.fdopen(_open(self._location), 'w+') as handle: - handle.write(content) + # Use a per-process unique temp file to avoid the shared-tmp race: if two processes + # (which both bypassed the lock) write to the same .tmp path concurrently, the shorter + # write leaves leftover bytes from the longer write, corrupting the JSON with "Extra data". + tmp = "{}.tmp.{}.{}".format(self._location, os.getpid(), os.urandom(4).hex()) + try: + with os.fdopen(os.open(tmp, os.O_RDWR | os.O_CREAT | os.O_TRUNC, 0o600), 'w+') as handle: + handle.write(content) + os.replace(tmp, self._location) + except Exception: + try: + os.unlink(tmp) + except OSError: + pass + raise def load(self): # type: () -> str