From 1e14f8998e51543aef4ff6ab5d00ec1dc428c89e Mon Sep 17 00:00:00 2001 From: Robbert Kouprie Date: Mon, 6 Jul 2026 16:15:19 +0200 Subject: [PATCH 1/3] fix: CrossPlatLock races that cause JSONDecodeError('Extra data') under concurrency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two races in the original CrossPlatLock design corrupted the shared MSAL token cache when many processes (e.g. 300+ parallel az-cli invocations) accessed it simultaneously: Race 1 — LOCK_NB bypasses the lock entirely portalocker.LOCK_EX | portalocker.LOCK_NB raises LockError immediately when any other process holds the lock. The caller catches LockError and continues without the lock, so all but the first concurrent process skip the critical section and proceed to write the cache simultaneously. Race 2 — lockfile deletion breaks POSIX inode-based exclusion __exit__ calls os.remove(lockfile). If process A deletes the file and process B immediately creates a new one at the same path, A and B now hold exclusive locks on *different inodes*. POSIX fcntl locks are per-(pid,inode) pair, so both hold a valid exclusive lock — mutual exclusion is lost. Fix: - Replace LOCK_EX | LOCK_NB with blocking LOCK_EX only. All concurrent processes now queue up and take turns; none bypass the lock. - Open the lockfile in append mode ('a') so it is never truncated. This preserves the existing inode across the lifetime of the process group. - Never delete the lockfile on __exit__. The file remains on disk across invocations; all processes open the same inode, so POSIX exclusion works correctly for the full session. --- msal_extensions/cache_lock.py | 63 ++++++++++++----------------------- 1 file changed, 22 insertions(+), 41 deletions(-) diff --git a/msal_extensions/cache_lock.py b/msal_extensions/cache_lock.py index e5ddf2b..27bc3fe 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,40 @@ 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). + # 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, buffering=0, ) - 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()) From 22f0df568a0666afbb35cf826f1f1600a599d560 Mon Sep 17 00:00:00 2001 From: Robbert Kouprie Date: Mon, 6 Jul 2026 16:15:47 +0200 Subject: [PATCH 2/3] fix: FilePersistence.save uses atomic per-process temp file to prevent cache corruption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Defense-in-depth fix for FilePersistence.save, complementing the CrossPlatLock fix in the previous commit. The original code wrote directly to the cache file path: with os.fdopen(_open(self._location), 'w+') as handle: handle.write(content) If two processes somehow bypass the lock (e.g. before the CrossPlatLock fix), both write to the *same* temp path (${location}.tmp). A shorter write arriving while a longer one is in progress leaves behind leftover bytes from the previous longer write. When the cache is later read back, json.loads() sees two JSON objects concatenated and raises: JSONDecodeError: Extra data: line N column C (char K) Fix: write to a per-process, per-invocation unique temp file (${location}.tmp.${pid}.${random_hex}) and then atomically rename it over the target with os.replace(). Because os.replace() is atomic on POSIX, readers always see either the old complete file or the new complete file — never a partial write. The temp file is cleaned up on any exception. --- msal_extensions/persistence.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) 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 From 71e2e6f8a1ec721d44e625052f3dd16c6d728dbc Mon Sep 17 00:00:00 2001 From: Robbert Kouprie Date: Mon, 6 Jul 2026 16:23:31 +0200 Subject: [PATCH 3/3] fix: remove buffering=0 from CrossPlatLock (invalid for text mode in Python 3) portalocker.Lock() defaults to mode='a' (text mode). Python 3 raises ValueError: can't have unbuffered text I/O when buffering=0 is passed with a text mode. Since the lockfile is never written to, buffering has no effect and can be removed entirely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- msal_extensions/cache_lock.py | 1 - 1 file changed, 1 deletion(-) diff --git a/msal_extensions/cache_lock.py b/msal_extensions/cache_lock.py index 27bc3fe..4c9c096 100644 --- a/msal_extensions/cache_lock.py +++ b/msal_extensions/cache_lock.py @@ -37,7 +37,6 @@ def __init__(self, lockfile_path): # 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, - buffering=0, ) def __enter__(self):