From 837be153e13e70e23ea5bb340d750b659732cc52 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Wed, 9 Sep 2026 01:12:25 +0200 Subject: [PATCH 01/24] fix: point deprecation warnings at the caller The maxval, poll, and currval deprecation warnings used stacklevel=1, so they named progressbar's own source instead of the line that used the old name. A fixed stacklevel=2 would only be right for currval: _apply_deprecated_aliases is reached through ProgressBar.__init__ and any subclass __init__ chain above it, so the depth varies per caller. _caller_stacklevel() walks the frames from the warning site outward until the code is no longer inside the progressbar package and returns that depth. Five tests record the warning and assert its filename and line are the caller's, for a plain bar, a DataTransferBar, a user subclass with its own __init__, and the currval property. --- progressbar/bar.py | 28 ++++++++- tests/test_deprecation_warnings.py | 95 ++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 tests/test_deprecation_warnings.py diff --git a/progressbar/bar.py b/progressbar/bar.py index 3cb8d963..6bf2d268 100644 --- a/progressbar/bar.py +++ b/progressbar/bar.py @@ -41,6 +41,28 @@ ) from .terminal import os_specific +_PACKAGE_DIR: str = os.path.dirname(os.path.abspath(__file__)) + + +def _caller_stacklevel() -> int: + """Return the `stacklevel` that points a warning at the first frame + outside progressbar. + + `warnings.warn()` counts level 1 as the function that calls it, so the + walk starts at this helper's caller and climbs until the code is not + ours. A fixed level would break as soon as a subclass adds an + `__init__` between the user and `ProgressBar.__init__`. + """ + frame: FrameType | None = sys._getframe(1) + level: int = 1 + while frame is not None and frame.f_code.co_filename.startswith( + _PACKAGE_DIR + ): + frame = frame.f_back + level += 1 + return level + + try: # Optional native accelerator, shipped as the ``progressbar2[fast]`` extra # (the separate ``speedups`` package). When importable, the iterator path @@ -904,7 +926,7 @@ def _apply_deprecated_aliases( 'The usage of `maxval` is deprecated, please use ' '`max_value` instead', DeprecationWarning, - stacklevel=1, + stacklevel=_caller_stacklevel(), ) max_value = kwargs.get('maxval') @@ -913,7 +935,7 @@ def _apply_deprecated_aliases( 'The usage of `poll` is deprecated, please use ' '`poll_interval` instead', DeprecationWarning, - stacklevel=1, + stacklevel=_caller_stacklevel(), ) poll_interval = kwargs.get('poll') @@ -1738,7 +1760,7 @@ def currval(self) -> NumberT: warnings.warn( 'The usage of `currval` is deprecated, please use `value` instead', DeprecationWarning, - stacklevel=1, + stacklevel=_caller_stacklevel(), ) return self.value diff --git a/tests/test_deprecation_warnings.py b/tests/test_deprecation_warnings.py new file mode 100644 index 00000000..9dfcdb5d --- /dev/null +++ b/tests/test_deprecation_warnings.py @@ -0,0 +1,95 @@ +"""The deprecation warnings must point at the caller, not at progressbar. + +A warning attributed to `progressbar/bar.py` tells the user nothing about +which of their own lines used the deprecated name. Each test below records +the warning and checks that it lands in this file, on the line that used +`maxval`, `poll` or `currval`, whatever the depth of the `__init__` chain +in between. +""" + +from __future__ import annotations + +import sys +import typing +import warnings + +import progressbar + + +def _line_before() -> int: + """Return the line number of the statement above the calling line.""" + return sys._getframe(1).f_lineno - 1 + + +def _single_deprecation( + caught: list[warnings.WarningMessage], +) -> warnings.WarningMessage: + assert len(caught) == 1 + assert caught[0].category is DeprecationWarning + return caught[0] + + +class _BarWithOwnInit(progressbar.ProgressBar): + """A user subclass that adds one frame between the caller and the bar.""" + + super_call_line: int + + def __init__(self, **kwargs: typing.Any) -> None: + super().__init__(**kwargs) + self.super_call_line = _line_before() + + +def test_maxval_warning_points_at_caller() -> None: + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always', DeprecationWarning) + progressbar.ProgressBar(maxval=10) + expected_line: int = _line_before() + + warning: warnings.WarningMessage = _single_deprecation(caught) + assert warning.filename == __file__ + assert warning.lineno == expected_line + + +def test_poll_warning_points_at_caller() -> None: + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always', DeprecationWarning) + progressbar.ProgressBar(poll=1) + expected_line: int = _line_before() + + warning: warnings.WarningMessage = _single_deprecation(caught) + assert warning.filename == __file__ + assert warning.lineno == expected_line + + +def test_currval_warning_points_at_caller() -> None: + bar: progressbar.ProgressBar = progressbar.ProgressBar(max_value=10) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always', DeprecationWarning) + assert bar.currval == 0 + expected_line: int = _line_before() + + warning: warnings.WarningMessage = _single_deprecation(caught) + assert warning.filename == __file__ + assert warning.lineno == expected_line + + +def test_subclass_maxval_warning_points_at_caller() -> None: + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always', DeprecationWarning) + progressbar.DataTransferBar(maxval=10) + expected_line: int = _line_before() + + warning: warnings.WarningMessage = _single_deprecation(caught) + assert warning.filename == __file__ + assert warning.lineno == expected_line + + +def test_user_subclass_init_warning_points_at_super_call() -> None: + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always', DeprecationWarning) + bar: _BarWithOwnInit = _BarWithOwnInit(maxval=10) + + warning: warnings.WarningMessage = _single_deprecation(caught) + assert warning.filename == __file__ + assert warning.lineno == bar.super_call_line From 5924c0c6cec185e54996c1cb4ae352390de0e970 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 22 Oct 2026 20:21:21 +0200 Subject: [PATCH 02/24] Make StreamWrapper listener handling safe on free-threaded builds WrappingIO.write iterated the shared listener set while start_capturing and stop_capturing on another thread added or removed bars, which on 3.14t and 3.15t raised "Set changed size during iteration" tens of thousands of times per run and dropped bar updates. The listener set is now guarded by a small dedicated StreamWrapper.listeners_lock that every WrappingIO shares, held only around the add, remove and the snapshot in write(), never while calling out, so it cannot take part in a lock-order cycle with a logging handler or a locking target stream. The wider StreamWrapper.lock (reentrant) guards wrap, unwrap and capture transitions. Both classes pickle and deepcopy again by dropping the locks in __getstate__ and recreating them on load. StreamWrapper has class-level default locks so a subclass that skips super().__init__() keeps working. Unwrapping delivers the dropped wrapper's buffered text to the original stream and never raises for a stream that cannot take it (logged at DEBUG). What working code can notice: WrappingIO gains an optional lock= keyword (api_surface_snapshot regenerated), its default lock is a plain threading.Lock, and text that was buffered in a wrapper at unwrap time now reaches the original stream instead of being lost. --- progressbar/utils.py | 190 ++++++++++++++++------ tests/api_surface_snapshot.json | 2 +- tests/test_stream_security.py | 274 ++++++++++++++++++++++++++++++++ 3 files changed, 417 insertions(+), 49 deletions(-) create mode 100644 tests/test_stream_security.py diff --git a/progressbar/utils.py b/progressbar/utils.py index af9c6aec..271c192e 100644 --- a/progressbar/utils.py +++ b/progressbar/utils.py @@ -18,6 +18,7 @@ import os import re import sys +import threading import typing from collections.abc import Callable, Iterable, Iterator, Mapping from types import TracebackType @@ -183,6 +184,7 @@ class WrappingIO: target: base.IO capturing: bool listeners: set[_ProgressListener] + lock: threading.Lock needs_clear: bool = False def __init__( @@ -190,6 +192,7 @@ def __init__( target: base.IO, capturing: bool = False, listeners: set[_ProgressListener] | None = None, + lock: threading.Lock | None = None, ) -> None: """Wrap `target` so writes can be buffered while `capturing`. @@ -202,13 +205,33 @@ def __init__( write completes a line. Typically shared with `StreamWrapper.listeners` by the caller, so every wrapped stream notifies the same bars. + lock: The lock that guards the `listeners` set and nothing + else. `StreamWrapper` passes its `listeners_lock`, so + the snapshot taken in `write()` cannot race with + `start_capturing()` and `stop_capturing()` on another + thread (a real race on free-threaded builds). It is + never held while calling out, so it cannot take part + in a lock-order cycle with a logging handler or a + locking target stream. Defaults to a private lock. """ self.buffer = io.StringIO() self.target = target self.capturing = capturing - self.listeners = listeners or set() + self.listeners = listeners if listeners is not None else set() + self.lock = lock if lock is not None else threading.Lock() self.needs_clear = False + def __getstate__(self) -> dict[str, typing.Any]: + """Pickle everything but the lock, which is recreated on load.""" + state: dict[str, typing.Any] = self.__dict__.copy() + state.pop('lock', None) + return state + + def __setstate__(self, state: dict[str, typing.Any]) -> None: + """Restore the state with a fresh, private lock.""" + self.__dict__.update(state) + self.lock = threading.Lock() + def write(self, value: str) -> int: """Write `value`, buffering it in memory while `capturing`. @@ -230,7 +253,12 @@ def write(self, value: str) -> int: ret += self.buffer.write(value) if '\n' in value: # pragma: no branch self.needs_clear = True - for listener in self.listeners: # pragma: no branch + # Iterate a snapshot taken under the lock: a listener's + # update() may finish its bar, which removes it from this + # shared set, and another thread may be mutating it + with self.lock: + listeners = tuple(self.listeners) + for listener in listeners: # pragma: no branch listener.update() else: ret += self.target.write(value) @@ -374,10 +402,33 @@ class StreamWrapper: wrapped_stdout: int = 0 wrapped_stderr: int = 0 wrapped_logging: int = 0 - wrapped_excepthook: int = 0 + wrapped_excepthook: bool = False logging_handlers: list[tuple[logging.StreamHandler[base.IO], base.IO]] capturing: int = 0 listeners: set[_ProgressListener] + #: Guards every wrap/unwrap/capture transition. Reentrant, because + #: wrap_stdout() calls wrap_excepthook() and update_capturing() calls + #: flush() while holding it. The class-level default keeps a subclass + #: that skips `super().__init__()` working, as every counter above. + lock: threading.RLock = threading.RLock() + #: Guards `listeners` only. Shared with every `WrappingIO` this + #: wrapper installs, and never held while calling out, so a captured + #: `write()` on one thread cannot wait for `lock` (which is held + #: across flushes into the original stream, logging included). + listeners_lock: threading.Lock = threading.Lock() + + def __getstate__(self) -> dict[str, typing.Any]: + """Pickle everything but the locks, which are recreated on load.""" + state: dict[str, typing.Any] = self.__dict__.copy() + state.pop('lock', None) + state.pop('listeners_lock', None) + return state + + def __setstate__(self, state: dict[str, typing.Any]) -> None: + """Restore the state with fresh, unlocked locks.""" + self.__dict__.update(state) + self.lock = threading.RLock() + self.listeners_lock = threading.Lock() def __init__(self) -> None: """Capture the *current* `sys.stdout`/`sys.stderr` as "real". @@ -397,10 +448,12 @@ def __init__(self) -> None: self.wrapped_stdout = 0 self.wrapped_stderr = 0 self.wrapped_logging = 0 - self.wrapped_excepthook = 0 + self.wrapped_excepthook = False self.logging_handlers = [] self.capturing = 0 self.listeners = set() + self.lock = threading.RLock() + self.listeners_lock = threading.Lock() if env.env_flag('WRAP_STDOUT', default=False): # pragma: no cover self.wrap_stdout() @@ -416,11 +469,13 @@ def start_capturing(self, bar: _ProgressListener | None = None) -> None: when captured output completes a line. Omit to just bump the refcount without listening. """ - if bar: # pragma: no branch - self.listeners.add(bar) + with self.lock: + if bar: # pragma: no branch + with self.listeners_lock: + self.listeners.add(bar) - self.capturing += 1 - self.update_capturing() + self.capturing += 1 + self.update_capturing() def stop_capturing(self, bar: _ProgressListener | None = None) -> None: """Unregister `bar` and drop the shared capturing refcount. @@ -429,12 +484,15 @@ def stop_capturing(self, bar: _ProgressListener | None = None) -> None: bar: The listener to remove, if it was registered via `start_capturing`. """ - if bar: # pragma: no branch - with contextlib.suppress(KeyError): - self.listeners.remove(bar) + with self.lock: + if bar: # pragma: no branch + with self.listeners_lock, contextlib.suppress(KeyError): + self.listeners.remove(bar) - self.capturing -= 1 - self.update_capturing() + # Never go negative: an unmatched stop would otherwise make + # the next start_capturing() a no-op + self.capturing = max(self.capturing - 1, 0) + self.update_capturing() def update_capturing(self) -> None: # pragma: no cover """Propagate the capturing refcount to the wrapped streams. @@ -444,14 +502,15 @@ def update_capturing(self) -> None: # pragma: no cover as the last bar stops capturing, rather than sitting there until something else happens to trigger a flush. """ - if isinstance(self.stdout, WrappingIO): - self.stdout.capturing = self.capturing > 0 + with self.lock: + if isinstance(self.stdout, WrappingIO): + self.stdout.capturing = self.capturing > 0 - if isinstance(self.stderr, WrappingIO): - self.stderr.capturing = self.capturing > 0 + if isinstance(self.stderr, WrappingIO): + self.stderr.capturing = self.capturing > 0 - if self.capturing <= 0: - self.flush() + if self.capturing <= 0: + self.flush() def wrap(self, stdout: bool = False, stderr: bool = False) -> None: """Wrap `stdout` and/or `stderr`, per the given flags.""" @@ -475,16 +534,18 @@ def wrap_stdout(self) -> WrappingIO: The installed `WrappingIO`, the same instance on every call until it's fully unwrapped. """ - self.wrap_excepthook() + with self.lock: + self.wrap_excepthook() - if not self.wrapped_stdout: - self.stdout = sys.stdout = WrappingIO( # type: ignore - self.original_stdout, - listeners=self.listeners, - ) - self.wrapped_stdout += 1 + if not self.wrapped_stdout: + self.stdout = sys.stdout = WrappingIO( # type: ignore + self.original_stdout, + listeners=self.listeners, + lock=self.listeners_lock, + ) + self.wrapped_stdout += 1 - return sys.stdout # type: ignore + return sys.stdout # type: ignore def wrap_stderr(self) -> WrappingIO: """Install a `WrappingIO` over `sys.stderr`, or share it. @@ -496,16 +557,18 @@ def wrap_stderr(self) -> WrappingIO: The installed `WrappingIO`, the same instance on every call until it's fully unwrapped. """ - self.wrap_excepthook() + with self.lock: + self.wrap_excepthook() - if not self.wrapped_stderr: - self.stderr = sys.stderr = WrappingIO( # type: ignore - self.original_stderr, - listeners=self.listeners, - ) - self.wrapped_stderr += 1 + if not self.wrapped_stderr: + self.stderr = sys.stderr = WrappingIO( # type: ignore + self.original_stderr, + listeners=self.listeners, + lock=self.listeners_lock, + ) + self.wrapped_stderr += 1 - return sys.stderr # type: ignore + return sys.stderr # type: ignore def wrap_logging(self) -> None: """Retarget every `StreamHandler` in the logger tree to the wrapper. @@ -634,9 +697,10 @@ def _iter_loggers(self) -> types.Iterator[logging.Logger]: def unwrap_excepthook(self) -> None: """Restore the original `sys.excepthook`, if currently wrapped.""" - if self.wrapped_excepthook: - self.wrapped_excepthook -= 1 - sys.excepthook = self.original_excepthook + with self.lock: + if self.wrapped_excepthook: + self.wrapped_excepthook = False + sys.excepthook = self.original_excepthook def wrap_excepthook(self) -> None: """Install the shared excepthook that flushes buffered output. @@ -645,10 +709,11 @@ def wrap_excepthook(self) -> None: both call this unconditionally, and either one may already have wrapped it. """ - if not self.wrapped_excepthook: - logger.debug('wrapping excepthook') - self.wrapped_excepthook += 1 - sys.excepthook = self.excepthook + with self.lock: + if not self.wrapped_excepthook: + logger.debug('wrapping excepthook') + self.wrapped_excepthook = True + sys.excepthook = self.excepthook def unwrap(self, stdout: bool = False, stderr: bool = False) -> None: """Unwrap `stdout` and/or `stderr`, per the given flags.""" @@ -658,6 +723,25 @@ def unwrap(self, stdout: bool = False, stderr: bool = False) -> None: if stderr: self.unwrap_stderr() + @staticmethod + def _deliver_dropped(dropped: WrappingIO) -> None: + """Stop buffering in a dropped wrapper and deliver what it holds. + + A stale reference to the wrapper must not swallow output. The + delivery touches the original stream, so a stream that cannot + take it (no `flush()`, closed, a broken pipe, a GUI widget that + is gone) is logged and otherwise ignored: unwrapping never + raises for it, as it did not before the flush was added. + """ + dropped.capturing = False + try: + dropped._flush() + except Exception: + logger.debug( + 'Could not deliver buffered output on unwrap', + exc_info=True, + ) + def unwrap_stdout(self) -> None: """Undo one `wrap_stdout()` call, restoring at refcount zero. @@ -668,13 +752,18 @@ def unwrap_stdout(self) -> None: excepthook once `stderr` is back to its original too, since it's shared between the two. """ - if self.wrapped_stdout > 1: - self.wrapped_stdout -= 1 - else: + with self.lock: + if self.wrapped_stdout > 1: + self.wrapped_stdout -= 1 + return + + dropped: object = self.stdout # Also reset our own reference so needs_clear() and # update_capturing() don't act on a stale wrapper self.stdout = sys.stdout = self.original_stdout self.wrapped_stdout = 0 + if isinstance(dropped, WrappingIO): + self._deliver_dropped(dropped) if not self.wrapped_stderr: self.unwrap_excepthook() @@ -686,13 +775,18 @@ def unwrap_stderr(self) -> None: excepthook is unwrapped once `stdout` is back to its original too. """ - if self.wrapped_stderr > 1: - self.wrapped_stderr -= 1 - else: + with self.lock: + if self.wrapped_stderr > 1: + self.wrapped_stderr -= 1 + return + + dropped: object = self.stderr # Also reset our own reference so needs_clear() and # update_capturing() don't act on a stale wrapper self.stderr = sys.stderr = self.original_stderr self.wrapped_stderr = 0 + if isinstance(dropped, WrappingIO): + self._deliver_dropped(dropped) if not self.wrapped_stdout: self.unwrap_excepthook() diff --git a/tests/api_surface_snapshot.json b/tests/api_surface_snapshot.json index b254d0e1..0d8e6674 100644 --- a/tests/api_surface_snapshot.json +++ b/tests/api_surface_snapshot.json @@ -481,7 +481,7 @@ "StringT": "type-alias", "T": "type-alias", "TracebackType": "re-export", - "WrappingIO": "class(target, capturing=?, listeners=?)", + "WrappingIO": "class(target, capturing=?, listeners=?, lock=?)", "annotations": "_Feature", "deltas_to_seconds": "callable(*deltas, default=?)", "epoch": "datetime", diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py new file mode 100644 index 00000000..4d44e290 --- /dev/null +++ b/tests/test_stream_security.py @@ -0,0 +1,274 @@ +from __future__ import annotations + +import io +import sys +from collections.abc import Iterator + +import pytest +from test_stream import reset_wrapped_streams + +import progressbar +from progressbar import utils + + +@pytest.fixture(autouse=True) +def clean_streams() -> Iterator[None]: + reset_wrapped_streams() + yield + reset_wrapped_streams() + + +class _SelfRemovingListener: + def __init__(self, listeners: set[object]) -> None: + self.listeners: set[object] = listeners + self.calls: int = 0 + + def update(self) -> None: + self.calls += 1 + self.listeners.discard(self) + self.listeners.add(_NoopListener()) + + +class _NoopListener: + def update(self) -> None: + pass + + +def test_listener_mutation_during_write_does_not_raise() -> None: + target: io.StringIO = io.StringIO() + listeners: set[object] = set() + listener: _SelfRemovingListener = _SelfRemovingListener(listeners) + listeners.add(listener) + wrapper: utils.WrappingIO = utils.WrappingIO( + target, + capturing=True, + listeners=listeners, # type: ignore[arg-type] + ) + + wrapper.write('line\n') + + assert listener.calls == 1 + assert listener not in listeners + + +def test_empty_listener_set_is_shared() -> None: + listeners: set[object] = set() + wrapper: utils.WrappingIO = utils.WrappingIO( + io.StringIO(), + listeners=listeners, # type: ignore[arg-type] + ) + assert wrapper.listeners is listeners + + +def test_stop_capturing_never_goes_negative() -> None: + streams: utils.StreamWrapper = progressbar.streams + assert streams.capturing == 0 + streams.stop_capturing() + streams.stop_capturing() + assert streams.capturing == 0 + streams.start_capturing() + assert streams.capturing == 1 + streams.stop_capturing() + assert streams.capturing == 0 + + +def test_unwrap_while_capturing_flushes_dropped_wrapper( + monkeypatch: pytest.MonkeyPatch, +) -> None: + streams: utils.StreamWrapper = progressbar.streams + real: io.StringIO = io.StringIO() + monkeypatch.setattr(streams, 'original_stdout', real) + + wrapper: utils.WrappingIO = streams.wrap_stdout() + streams.start_capturing() + try: + assert wrapper.capturing + sys.stdout.write('held\n') + assert real.getvalue() == '' + + streams.unwrap_stdout() + + assert sys.stdout is real + assert not wrapper.capturing + assert wrapper.buffer.getvalue() == '' + assert real.getvalue() == 'held\n' + assert not streams.wrapped_excepthook + finally: + streams.stop_capturing() + + +def test_excepthook_flag_is_boolean() -> None: + streams: utils.StreamWrapper = progressbar.streams + streams.wrap_stdout() + streams.wrap_stderr() + assert streams.wrapped_excepthook is True + streams.unwrap_stdout() + assert streams.wrapped_excepthook is True + streams.unwrap_stderr() + assert streams.wrapped_excepthook is False + + +def test_unwrap_stdout_without_wrapper_restores_original() -> None: + streams: utils.StreamWrapper = progressbar.streams + assert not isinstance(streams.stdout, utils.WrappingIO) + streams.unwrap_stdout() + assert streams.wrapped_stdout == 0 + assert sys.stdout is streams.original_stdout + + +# ---- round-3 take-backs ------------------------------------------------- + + +def test_wrapper_snapshots_listeners_under_the_shared_lock() -> None: + streams: utils.StreamWrapper = progressbar.streams + wrapper: utils.WrappingIO = streams.wrap_stdout() + try: + assert wrapper.lock is streams.listeners_lock + finally: + streams.unwrap_stdout() + + +def test_standalone_wrapper_has_its_own_lock() -> None: + wrapper: utils.WrappingIO = utils.WrappingIO(io.StringIO()) + assert wrapper.lock.acquire(blocking=False) + wrapper.lock.release() + + +def test_streamwrapper_survives_deepcopy_and_pickle( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import copy + import pickle + + monkeypatch.setattr(sys, 'stdout', io.StringIO()) + monkeypatch.setattr(sys, 'stderr', io.StringIO()) + original: utils.StreamWrapper = utils.StreamWrapper() + clone: utils.StreamWrapper = copy.deepcopy(original) + assert clone.lock is not original.lock + assert clone.lock.acquire(blocking=False) + clone.lock.release() + restored: utils.StreamWrapper = pickle.loads(pickle.dumps(original)) + assert restored.lock.acquire(blocking=False) + restored.lock.release() + + +def test_unwrap_tolerates_a_stream_without_flush( + monkeypatch: pytest.MonkeyPatch, +) -> None: + class WriteOnly: + def write(self, text: str) -> int: + return len(text) + + streams: utils.StreamWrapper = progressbar.streams + target: WriteOnly = WriteOnly() + monkeypatch.setattr(streams, 'original_stdout', target) + monkeypatch.setattr(streams, 'stdout', target) + monkeypatch.setattr(sys, 'stdout', target) + streams.wrap_stdout() + streams.unwrap_stdout() # must not raise + assert sys.stdout is target + + +def test_subclass_without_super_init_can_wrap( + monkeypatch: pytest.MonkeyPatch, +) -> None: + class NoSuper(utils.StreamWrapper): + def __init__(self, out: io.StringIO) -> None: + self.stdout = self.original_stdout = out + self.stderr = self.original_stderr = io.StringIO() + self.original_excepthook = sys.excepthook + self.listeners = set() + self.logging_handlers = [] + + hook = sys.excepthook + wrapper: NoSuper = NoSuper(io.StringIO()) + try: + wrapper.wrap_stdout() + finally: + wrapper.unwrap_stdout() + monkeypatch.setattr(sys, 'excepthook', hook) + + +# ---- round-3 closing fixes --------------------------------------------- + + +def test_wrapping_io_and_a_redirecting_bar_deepcopy_and_pickle( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import copy + import pickle + + wrapper: utils.WrappingIO = utils.WrappingIO(io.StringIO()) + clone: utils.WrappingIO = copy.deepcopy(wrapper) + assert clone.lock is not wrapper.lock + assert clone.lock.acquire(blocking=False) + clone.lock.release() + restored: utils.WrappingIO = pickle.loads(pickle.dumps(wrapper)) + assert restored.lock.acquire(blocking=False) + restored.lock.release() + + streams: utils.StreamWrapper = progressbar.streams + target: io.StringIO = io.StringIO() + errors: io.StringIO = io.StringIO() + monkeypatch.setattr(streams, 'original_stdout', target) + monkeypatch.setattr(streams, 'stdout', target) + monkeypatch.setattr(sys, 'stdout', target) + monkeypatch.setattr(streams, 'original_stderr', errors) + monkeypatch.setattr(streams, 'stderr', errors) + monkeypatch.setattr(sys, 'stderr', errors) + bar: progressbar.ProgressBar = progressbar.ProgressBar( + max_value=5, redirect_stdout=True, fd=io.StringIO() + ) + bar.start() + try: + copy.deepcopy(bar) + copy.deepcopy(streams) # while wrapping + finally: + bar.finish() + + +def test_captured_write_never_waits_for_the_streamwrapper_lock() -> None: + import threading + + streams: utils.StreamWrapper = progressbar.streams + wrapper: utils.WrappingIO = streams.wrap_stdout() + try: + assert wrapper.lock is streams.listeners_lock + assert wrapper.lock is not streams.lock + streams.lock.acquire() + done: threading.Event = threading.Event() + + def write() -> None: + wrapper.capturing = True + wrapper.write('line\n') + done.set() + + thread: threading.Thread = threading.Thread(target=write) + thread.start() + assert done.wait(2.0), 'write() blocked on streams.lock' + thread.join() + finally: + streams.lock.release() + streams.unwrap_stdout() + + +def test_unwrap_tolerates_a_stream_whose_flush_raises_anything( + monkeypatch: pytest.MonkeyPatch, +) -> None: + class DeletedWidget: # PyQt 'wrapped C/C++ object has been deleted' shape + closed: bool = False + + def write(self, text: str) -> int: + return len(text) + + def flush(self) -> None: + raise RuntimeError('wrapped C/C++ object has been deleted') + + streams: utils.StreamWrapper = progressbar.streams + target: DeletedWidget = DeletedWidget() + monkeypatch.setattr(streams, 'original_stdout', target) + monkeypatch.setattr(streams, 'stdout', target) + monkeypatch.setattr(sys, 'stdout', target) + streams.wrap_stdout() + streams.unwrap_stdout() # must not raise + assert sys.stdout is target From fded969d4fa27496bf7507268074f7facbaeb445 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 22 Oct 2026 20:21:22 +0200 Subject: [PATCH 03/24] Stop format templates from reaching dunder attributes The new-style widget formatter resolved any attribute path a template named, so a template taken from user input could walk {variables.x.__class__} and further into the interpreter. Rendering goes through a string.Formatter subclass that refuses attribute segments starting with two underscores and raises ValueError; single-underscore names and everything else render exactly as in 4.6.0. Non-str templates with a callable format() (lazy translation proxies) are rendered through str(); None, lists and numbers raise TypeError as before. ProgressBar(desc=...) is rendered literally as f'{desc}: ' and is never a format template; a prefix assigned afterwards is a template as before. PROGRESSBAR_MINIMUM_UPDATE_INTERVAL that is not a non-negative finite number is ignored with one WARNING per distinct value instead of raising at construction. What working code can notice: templates addressing __dunder__ paths now raise ValueError, a desc containing braces renders them literally, and a bad interval variable logs instead of raising. --- progressbar/bar.py | 49 +++++++- progressbar/widgets.py | 67 ++++++++++- tests/test_format_security.py | 206 ++++++++++++++++++++++++++++++++++ 3 files changed, 311 insertions(+), 11 deletions(-) create mode 100644 tests/test_format_security.py diff --git a/progressbar/bar.py b/progressbar/bar.py index 6bf2d268..46e61ada 100644 --- a/progressbar/bar.py +++ b/progressbar/bar.py @@ -76,6 +76,36 @@ def _caller_stacklevel() -> int: _FastBarIterator = None +@functools.lru_cache(maxsize=16) +def _warn_bad_minimum_update_interval(raw: str) -> None: + """Warn once per distinct bad value, not once per bar.""" + logger.warning( + 'Ignoring PROGRESSBAR_MINIMUM_UPDATE_INTERVAL=%r: ' + 'not a non-negative finite number', + raw, + ) + + +def _env_minimum_update_interval() -> float: + """Read `PROGRESSBAR_MINIMUM_UPDATE_INTERVAL`, ignoring bad values. + + A non-numeric, negative or non-finite value is logged as a warning + and falls back to 0, so a stray environment variable cannot stop a + bar from being built. An unset or empty variable is 0 silently. + """ + raw: str = os.environ.get('PROGRESSBAR_MINIMUM_UPDATE_INTERVAL', '') + if not raw.strip(): + return 0.0 + try: + value: float = float(raw) + except ValueError: + value = -1.0 + if not math.isfinite(value) or value < 0: + _warn_bad_minimum_update_interval(raw) + return 0.0 + return value + + @functools.cache def _load_widgets() -> typing.Any: """Import the widgets module lazily (and once). @@ -786,7 +816,9 @@ class ProgressBar( line_offset: The number of lines to offset the progressbar from your current line. This is useful if you have other output or multiple progressbars. - desc: tqdm-style alias for `prefix` (rendered as `f'{desc}: '`). + desc: tqdm-style description, rendered literally as + `f'{desc}: '` in front of the bar. Unlike `prefix` it is + never a format template: braces in a file name stay braces. Ignored if `prefix` is also given. total: tqdm-style alias for `max_value`. Ignored if `max_value` is also given. @@ -872,8 +904,11 @@ def __init__( # tqdm-style alias for `max_value` max_value = total if prefix is None and desc is not None: - # tqdm-style alias for `prefix` + # tqdm-style alias for `prefix`: `desc` is literal text, as in + # tqdm and as the fast path renders it. Remember the exact + # string, so a prefix assigned later is a template again. prefix = f'{desc}: ' + self._literal_prefix: str | None = prefix if max_value and min_value > typing.cast(NumberT, max_value): raise ValueError( @@ -986,7 +1021,7 @@ def _setup_poll_intervals( self.min_poll_interval = max( min_poll_interval or self._MINIMUM_UPDATE_INTERVAL, self._MINIMUM_UPDATE_INTERVAL, - float(os.environ.get('PROGRESSBAR_MINIMUM_UPDATE_INTERVAL', 0)), + _env_minimum_update_interval(), ) # type: ignore def _seed_variables(self, variables: dict[str, typing.Any] | None) -> None: @@ -1687,10 +1722,12 @@ def _init_prefix(self) -> None: if self.prefix: widgets = _load_widgets() - self.widgets.insert( - 0, - widgets.FormatLabel(self.prefix, new_style=True), + label: typing.Any = ( + self.prefix + if self.prefix is getattr(self, '_literal_prefix', None) + else widgets.FormatLabel(self.prefix, new_style=True) ) + self.widgets.insert(0, label) self.prefix = None def _verify_max_value(self) -> None: diff --git a/progressbar/widgets.py b/progressbar/widgets.py index ed3cc40d..8d5e2b58 100644 --- a/progressbar/widgets.py +++ b/progressbar/widgets.py @@ -8,12 +8,14 @@ from __future__ import annotations +import _string # type: ignore[import-not-found] import abc import collections.abc import contextlib import datetime import functools import logging +import string import typing from typing import ClassVar @@ -27,6 +29,61 @@ logger = logging.getLogger(__name__) + +class _SafeFormatter(string.Formatter): + """`str.format` over widget data that refuses dunder attribute walks. + + Format templates may come from text the application did not write + (a `prefix`, a `Variable` format). A field such as + `{variables.__class__.__init__.__globals__}` would otherwise reach + module globals, so any attribute segment starting with `__` raises + `ValueError` before it is looked up. A single leading underscore is + allowed: `{variables._my_var}` is a documented way to reach a + variable, and a user object's own `_private` attribute is the + user's business. + """ + + def get_field( + self, + field_name: str, + args: typing.Sequence[typing.Any], + kwargs: typing.Mapping[str, typing.Any], + ) -> tuple[typing.Any, str]: + rest: typing.Iterable[tuple[bool, typing.Any]] + _first, rest = _string.formatter_field_name_split(field_name) + for is_attr, name in rest: + if is_attr and str(name).startswith('__'): + raise ValueError( + f'attribute {name!r} is not allowed in a format template', + ) + return super().get_field(field_name, args, kwargs) + + +_SAFE_FORMATTER: _SafeFormatter = _SafeFormatter() + + +def _safe_format( + template: object, + data: typing.Mapping[str, typing.Any], +) -> str: + """Render `template` with `data` through the safe formatter. + + `string.Formatter` needs a real `str`, while `str.format` accepted + anything with a `format()` method. A translation proxy (the + `gettext_lazy` shape) is therefore rendered through `str()` first, + so it keeps working. Anything else that is not text (None, a list, + a number) is a `TypeError`, as it was before. + """ + if not isinstance(template, str): + if not callable(getattr(template, 'format', None)): + raise TypeError( + 'format template must be a str, got ' + f'{type(template).__name__}', + ) + template = str(template) + return _SAFE_FORMATTER.vformat(template, (), data) + + MAX_DATE = datetime.date.max MAX_TIME = datetime.time.max MAX_DATETIME = datetime.datetime.max @@ -198,14 +255,14 @@ def __call__( format_ = self.get_format(progress, data, format) try: if self.new_style: - return format_.format(**data) + return _safe_format(format_, data) else: return format_ % data - except (TypeError, KeyError): + except (TypeError, KeyError, IndexError, ValueError): logger.exception( - 'Error while formatting %r with data: %r', + 'Error while formatting %r with data keys: %r', format_, - data, + list(data), ) raise @@ -2072,7 +2129,7 @@ def __call__( else: context['formatted_value'] = '-' * self.width - return self.format.format(**context) + return _safe_format(self.format, context) class DynamicMessage(Variable): diff --git a/tests/test_format_security.py b/tests/test_format_security.py new file mode 100644 index 00000000..27bca3ff --- /dev/null +++ b/tests/test_format_security.py @@ -0,0 +1,206 @@ +"""Format templates must not walk private attributes or crash on desc.""" + +from __future__ import annotations + +import io + +import pytest + +import progressbar + + +def _render(**kwargs: object) -> str: + fd: io.StringIO = io.StringIO() + bar: progressbar.ProgressBar = progressbar.ProgressBar( + fd=fd, + max_value=10, + term_width=200, + **kwargs, # type: ignore[arg-type] + ) + bar.start() + bar.update(5) + bar.finish() + return fd.getvalue() + + +def test_desc_attribute_walk_renders_literally() -> None: + desc: str = ( + '{variables.__class__.__getattr__.__globals__[os].environ[HOME]}' + ) + output: str = _render(desc=desc) + assert desc + ': ' in output + + +def test_desc_with_braces_renders_literally() -> None: + output: str = _render(desc='report{2024}.txt') + assert 'report{2024}.txt: ' in output + + +def test_prefix_private_attribute_raises() -> None: + with pytest.raises(ValueError, match='__class__'): + _render(prefix='{variables.__class__}') + + +def test_prefix_value_template_still_renders() -> None: + output: str = _render(prefix='{value} ') + assert '10 ' in output + + +def test_prefix_variable_template_still_renders() -> None: + output: str = _render( + prefix='{variables.name} ', + variables={'name': 'spam'}, + ) + assert 'spam ' in output + + +def test_variable_format_private_attribute_raises() -> None: + widget: progressbar.Variable = progressbar.Variable( + 'name', + format='{formatted_value.__class__}', + ) + with pytest.raises(ValueError, match='__class__'): + _render(widgets=[widget], variables={'name': 'eggs'}) + + +def test_format_error_logs_keys_not_values( + caplog: pytest.LogCaptureFixture, +) -> None: + with pytest.raises(KeyError): + _render(prefix='{missing} ', variables={'secret': 'hunter2'}) + assert 'hunter2' not in caplog.text + + +@pytest.mark.parametrize('raw', ['', 'inf', 'nan', 'spam', '-1']) +def test_bad_minimum_update_interval_env_is_ignored( + monkeypatch: pytest.MonkeyPatch, + raw: str, +) -> None: + monkeypatch.setenv('PROGRESSBAR_MINIMUM_UPDATE_INTERVAL', raw) + from progressbar import bar as bar_module + + assert bar_module._env_minimum_update_interval() == 0.0 + output: str = _render() + assert output + + +def test_valid_minimum_update_interval_env_is_used( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setenv('PROGRESSBAR_MINIMUM_UPDATE_INTERVAL', '0.5') + from progressbar import bar as bar_module + + assert bar_module._env_minimum_update_interval() == 0.5 + + +# ---- round-3 take-backs ------------------------------------------------- + + +class _LazyStr: + """A translation proxy shape: not a str, but has format() and __str__.""" + + def __init__(self, text: str) -> None: + self.text: str = text + + def format(self, *args: object, **kwargs: object) -> str: + return self.text.format(*args, **kwargs) + + def __str__(self) -> str: + return self.text + + +def _label_data() -> tuple[progressbar.ProgressBar, dict[str, object]]: + bar: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO(), max_value=10, variables={'_x': 'v1', 'x': 1} + ) + bar.start() + bar.update(3, _x='v2') + return bar, dict(bar.data()) + + +def test_lazy_template_object_renders() -> None: + bar, data = _label_data() + label = progressbar.FormatLabel( + _LazyStr('{value} of {max_value}'), # type: ignore[arg-type] + new_style=True, + ) + assert label(bar, data) == '3 of 10' + + +def test_single_underscore_variable_renders() -> None: + bar, data = _label_data() + label = progressbar.FormatLabel('{variables._x}', new_style=True) + assert label(bar, data) == 'v2' + + +def test_dunder_attribute_still_refused() -> None: + bar, data = _label_data() + label = progressbar.FormatLabel('{variables.x.__class__}', new_style=True) + with pytest.raises(ValueError, match='__class__'): + label(bar, data) + + +def test_prefix_reassigned_after_desc_is_a_template() -> None: + fd: io.StringIO = io.StringIO() + bar: progressbar.ProgressBar = progressbar.ProgressBar( + fd=fd, max_value=10, desc='d', widgets=[progressbar.SimpleProgress()] + ) + bar.prefix = 'late {value} ' + bar.start() + bar.update(4, force=True) + assert 'late 4 4 of 10' in fd.getvalue() + assert '{value}' not in fd.getvalue() + + +def test_format_error_with_non_str_keys_raises_the_template_error() -> None: + bar, data = _label_data() + data[0] = 'zero' # type: ignore[index] + label = progressbar.FormatLabel('{nope}', new_style=True) + with pytest.raises(KeyError, match='nope'): + label(bar, data) + + +def test_bad_minimum_update_interval_env_logs_a_warning( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + import logging + + from progressbar import bar as bar_module + + bar_module._warn_bad_minimum_update_interval.cache_clear() + monkeypatch.setenv('PROGRESSBAR_MINIMUM_UPDATE_INTERVAL', 'spam') + with caplog.at_level(logging.WARNING, logger='progressbar.bar'): + progressbar.ProgressBar(fd=io.StringIO(), max_value=10) + assert any( + 'PROGRESSBAR_MINIMUM_UPDATE_INTERVAL' in record.getMessage() + and 'spam' in record.getMessage() + for record in caplog.records + ) + + +def test_non_text_template_without_format_raises() -> None: + bar, data = _label_data() + + class NotCallableFormat: + format = 'text' + + for template in (None, ['{value}'], 42, NotCallableFormat()): + label = progressbar.FormatLabel(template, new_style=True) # type: ignore[arg-type] + with pytest.raises(TypeError): + label(bar, data) + + +def test_bad_minimum_update_interval_env_warns_once_per_value( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + import logging + + from progressbar import bar as bar_module + + bar_module._warn_bad_minimum_update_interval.cache_clear() + monkeypatch.setenv('PROGRESSBAR_MINIMUM_UPDATE_INTERVAL', 'eggs') + with caplog.at_level(logging.WARNING, logger='progressbar.bar'): + for _ in range(3): + progressbar.ProgressBar(fd=io.StringIO(), max_value=10) + records = [r for r in caplog.records if 'eggs' in r.getMessage()] + assert len(records) == 1 From 77b29a3cc00b4aea7c4936c30e0cfce4233539c8 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 22 Oct 2026 20:21:22 +0200 Subject: [PATCH 04/24] Harden the pv-style command line copy Validate every size option through size_to_bytes with a minimum and a parser error (status 2) instead of a traceback, reject a missing input, a directory input, a directory output and an output path that is also an input before the output is truncated, and close every input after its copy. An early-closing reader (`... | head`) ends the copy quietly with status 0: the BrokenPipeError is caught and, when the broken stream is the process's real stdout (fd 1), the descriptor is pointed at os.devnull so the interpreter's exit flush cannot raise again. A replacement sys.stdout that owns another descriptor is left alone. Ctrl-C exits 130. main() is split into small helpers (_parse_size_option, _collect_inputs, _check_output_path, _open_input, _copy_stream) with no behaviour change beyond the above. What working code can notice: invalid -s/-B/-L values exit 2 with a message instead of a traceback, `-o` onto an input file is refused, and `... | head` exits 0 where 4.6.0 exited 120 with "Exception ignored". --- progressbar/__main__.py | 305 +++++++++++++++++++++--------- tests/test_progressbar_command.py | 230 +++++++++++++++++++++- 2 files changed, 447 insertions(+), 88 deletions(-) diff --git a/progressbar/__main__.py b/progressbar/__main__.py index f271b08f..d64d4c25 100644 --- a/progressbar/__main__.py +++ b/progressbar/__main__.py @@ -9,12 +9,13 @@ import argparse import contextlib +import os import pathlib import sys import time import typing from pathlib import Path -from typing import IO, BinaryIO, TextIO +from typing import IO import progressbar @@ -386,118 +387,248 @@ def _sleep_for_rate_limit( time.sleep(delay) -def main(argv: list[str] | None = None) -> None: # noqa: C901 +#: Exit status after an interrupted transfer (128 + SIGINT, as shells do). +EXIT_INTERRUPTED: int = 130 + + +def _parse_size_option( + parser: argparse.ArgumentParser, + option: str, + value: str, + minimum: int, +) -> int: + """Parse a size option, exiting through `parser.error` when invalid. + + Args: + parser: The parser used to report the error (exit status 2). + option: The flag name, used in the error message. + value: The raw option value. + minimum: The smallest accepted value in bytes. + + Returns: + The size in bytes. + """ + try: + size: int = size_to_bytes(value) + except (ValueError, IndexError, OSError) as exception: + parser.error(f'invalid {option} value {value!r}: {exception}') + if size < minimum: + parser.error(f'{option} must be at least {minimum}, got {value!r}') + return size + + +def _collect_inputs( + parser: argparse.ArgumentParser, + args: argparse.Namespace, +) -> tuple[list[IO[typing.Any] | Path], int, bool]: + """Validate the input arguments without opening any file. + + Returns: + The inputs (stdin streams or paths), the summed file size and + whether a total size is known. + """ + inputs: list[IO[typing.Any] | Path] = [] + total_size: int = 0 + filesize_available: bool = True + for filename in args.input: + if filename == '-': + inputs.append(sys.stdin if args.line_mode else sys.stdin.buffer) + filesize_available = False + continue + + input_path: Path = pathlib.Path(filename) + if not input_path.exists(): + parser.error(f'File not found: {filename}') + if input_path.is_dir(): + parser.error(f'Is a directory: {filename}') + total_size += input_path.stat().st_size + inputs.append(input_path) + return inputs, total_size, filesize_available + + +def _check_output_path( + parser: argparse.ArgumentParser, + output: str | None, + inputs: typing.Sequence[IO[typing.Any] | Path], +) -> None: + """Reject an output path that is a directory or one of the inputs. + + Runs before the output is opened, because opening it truncates it. + """ + if not output or output == '-': + return + output_path: Path = pathlib.Path(output) + if not output_path.exists(): + return + if output_path.is_dir(): + parser.error(f'Output is a directory: {output}') + if not output_path.is_file(): + # Devices (two names of one tty, /dev/null) share an inode without + # sharing data, so only a regular file can be truncated away + return + for input_path in inputs: + if isinstance(input_path, Path) and output_path.samefile(input_path): + parser.error(f'Output file is also an input: {output}') + + +def _open_input( + input_path: IO[typing.Any] | Path, + line_mode: bool, +) -> typing.ContextManager[IO[typing.Any]]: + """Open a path input, or wrap a stdin stream without closing it.""" + if isinstance(input_path, Path): + if line_mode: + # newline='' disables universal-newline translation so the + # byte count matches the file size for CRLF files as well + return input_path.open('r', newline='') + return input_path.open('rb') + return contextlib.nullcontext(input_path) + + +def _copy_stream( + input_stream: IO[typing.Any], + output_stream: IO[typing.Any], + bar: progressbar.ProgressBar, + line_mode: bool, + buffer_size: int, + rate_limit: int | None, + started_at: float, + transferred: int, +) -> int: + """Copy one input to the output, updating `bar`. + + Returns: + The running total of bytes transferred. + """ + while True: + data: str | bytes + if line_mode: + data = input_stream.readline(buffer_size) + else: + data = input_stream.read(buffer_size) + + if not data: + return transferred + + output_stream.write(data) + if isinstance(data, str): + # The total size is measured in bytes, so progress must be + # tracked in bytes as well + encoding: str = getattr(input_stream, 'encoding', None) or 'utf-8' + transferred += len(data.encode(encoding, errors='replace')) + else: + transferred += len(data) + + bar.update(transferred) + _sleep_for_rate_limit(rate_limit, transferred, started_at) + + +def _discard_broken_stdout() -> None: + """Point fd 1 at `os.devnull` after a broken pipe on stdout. + + The copy loop has already stopped, but the interpreter flushes + `sys.stdout` once more at exit, and that flush would raise the same + `BrokenPipeError` again (status 120, "Exception ignored on flushing + sys.stdout"). Redirecting the descriptor makes that last flush + succeed, which is what the Python docs recommend for a pipeline + whose reader went away. + """ + try: + fd: int = sys.stdout.fileno() + except (OSError, ValueError, AttributeError): + return # not descriptor-backed (io.StringIO under a test runner) + if fd != 1: + return # a replacement stdout owns that descriptor, leave it alone + try: + devnull: int = os.open(os.devnull, os.O_WRONLY) + try: + os.dup2(devnull, fd) + finally: + os.close(devnull) + except OSError: # pragma: no cover + pass + + +def main(argv: list[str] | None = None) -> None: """Run the `progressbar` command. + All arguments are validated before the output file is opened, so an + invalid invocation exits with status 2 and leaves `-o` untouched. An + interrupted transfer (Ctrl-C) exits with status 130 after the bar + is finished. A downstream pipe that closes early ends the copy + quietly with status 0, so `... | head` stays clean under pipefail. + Args: argv: Command-line arguments. Defaults to `sys.argv[1:]`. """ parser: argparse.ArgumentParser = create_argument_parser() args: argparse.Namespace = parser.parse_args(argv) + inputs, total_size, filesize_available = _collect_inputs(parser, args) + if args.size: + total_size = _parse_size_option(parser, '--size', args.size, 0) + filesize_available = True + buffer_size: int = ( + _parse_size_option(parser, '--buffer-size', args.buffer_size, 1) + if args.buffer_size + else 1024 + ) + rate_limit: int | None = ( + _parse_size_option(parser, '--rate-limit', args.rate_limit, 0) + if args.rate_limit + else None + ) + _check_output_path(parser, args.output, inputs) + + widgets = _build_widgets(args, filesize_available) + progress_bar_class: type[progressbar.ProgressBar] = ( + progressbar.NullBar if args.quiet else progressbar.ProgressBar + ) + exit_status: int = 0 + with contextlib.ExitStack() as stack: output_stream: typing.IO[typing.Any] = _get_output_stream( args.output, args.line_mode, stack ) - - input_paths: list[BinaryIO | TextIO | Path | IO[typing.Any]] = [] - total_size: int = 0 - filesize_available: bool = True - for filename in args.input: - input_path: typing.IO[typing.Any] | pathlib.Path - if filename == '-': - if args.line_mode: - input_path = sys.stdin - else: - input_path = sys.stdin.buffer - - filesize_available = False - else: - input_path = pathlib.Path(filename) - if not input_path.exists(): - parser.error(f'File not found: {filename}') - - if not args.size: - total_size += input_path.stat().st_size - - input_paths.append(input_path) - - # Determine the size for the progress bar (if provided) - if args.size: - total_size = size_to_bytes(args.size) - filesize_available = True - - widgets = _build_widgets(args, filesize_available) - progress_bar_class: type[progressbar.ProgressBar] = ( - progressbar.NullBar if args.quiet else progressbar.ProgressBar - ) - bar = progress_bar_class( widgets=widgets, max_value=total_size if filesize_available else None, max_error=False, line_breaks=True if args.numeric else None, ) - - buffer_size = ( - size_to_bytes(args.buffer_size) if args.buffer_size else 1024 - ) - rate_limit = ( - size_to_bytes(args.rate_limit) if args.rate_limit else None - ) - started_at = time.monotonic() - total_transferred = 0 + started_at: float = time.monotonic() + transferred: int = 0 bar.start() - with contextlib.suppress(KeyboardInterrupt, BrokenPipeError): - for input_path in input_paths: - if isinstance(input_path, pathlib.Path): - if args.line_mode: - # newline='' disables universal-newline - # translation so the byte count matches the file - # size for CRLF files as well - input_stream = stack.enter_context( - input_path.open('r', newline=''), - ) - else: - input_stream = stack.enter_context( - input_path.open('rb'), - ) - else: - input_stream = input_path - - while True: - data: str | bytes - if args.line_mode: - data = input_stream.readline(buffer_size) - else: - data = input_stream.read(buffer_size) - - if not data: - break - - output_stream.write(data) - if isinstance(data, str): - # The total size is measured in bytes, so progress - # must be tracked in bytes as well - encoding = ( - getattr(input_stream, 'encoding', None) or 'utf-8' - ) - total_transferred += len( - data.encode(encoding, errors='replace'), - ) - else: - total_transferred += len(data) - - bar.update(total_transferred) - _sleep_for_rate_limit( + try: + for input_path in inputs: + with _open_input(input_path, args.line_mode) as input_stream: + transferred = _copy_stream( + input_stream, + output_stream, + bar, + args.line_mode, + buffer_size, rate_limit, - total_transferred, started_at, + transferred, ) + except KeyboardInterrupt: + exit_status = EXIT_INTERRUPTED + except BrokenPipeError: + # The reader went away (`... | head`): finish quietly with + # status 0, as 4.6.0 did and as a pipeline under pipefail + # expects + if output_stream is sys.stdout or output_stream is getattr( + sys.stdout, 'buffer', None + ): + _discard_broken_stdout() bar.finish(dirty=True) + if exit_status: + raise SystemExit(exit_status) + def _get_output_stream( output: str | None, diff --git a/tests/test_progressbar_command.py b/tests/test_progressbar_command.py index 65807abe..b991bd4a 100644 --- a/tests/test_progressbar_command.py +++ b/tests/test_progressbar_command.py @@ -278,7 +278,7 @@ def write(self, data) -> int: monkeypatch.setattr( main, '_get_output_stream', lambda *args: BrokenPipeIO() ) - main.main([str(file)]) # must not raise + main.main([str(file)]) # must not raise, and must not exit non-zero def test_main_empty_file_has_known_size(tmp_path, recorded_bars) -> None: @@ -301,3 +301,231 @@ def test_console_scripts_include_the_bar_shorthand() -> None: assert "progressbar = 'progressbar.__main__:main'" in pyproject assert "bar = 'progressbar.__main__:main'" in pyproject + + +def test_missing_input_keeps_existing_output(tmp_path: pathlib.Path) -> None: + output: pathlib.Path = tmp_path / 'out.bin' + output.write_bytes(b'precious') + with pytest.raises(SystemExit) as excinfo: + main.main([str(tmp_path / 'missing'), '-o', str(output)]) + assert excinfo.value.code == 2 + assert output.read_bytes() == b'precious' + + +def test_directory_input_is_rejected(tmp_path: pathlib.Path) -> None: + output: pathlib.Path = tmp_path / 'out.bin' + output.write_bytes(b'precious') + with pytest.raises(SystemExit) as excinfo: + main.main([str(tmp_path), '-o', str(output)]) + assert excinfo.value.code == 2 + assert output.read_bytes() == b'precious' + + +def test_output_same_as_input_is_rejected(tmp_path: pathlib.Path) -> None: + file: pathlib.Path = tmp_path / 'data.bin' + file.write_bytes(b'precious') + with pytest.raises(SystemExit) as excinfo: + main.main([str(file), '-o', str(file)]) + assert excinfo.value.code == 2 + assert file.read_bytes() == b'precious' + + +@pytest.mark.parametrize( + 'option', [['-B', '0'], ['-B', 'abc'], ['-s', 'x'], ['-L', 'abc']] +) +def test_invalid_size_options_are_rejected( + tmp_path: pathlib.Path, option: list[str] +) -> None: + file: pathlib.Path = tmp_path / 'data.bin' + file.write_bytes(b'x' * 16) + output: pathlib.Path = tmp_path / 'out.bin' + output.write_bytes(b'precious') + with pytest.raises(SystemExit) as excinfo: + main.main([*option, str(file), '-o', str(output)]) + assert excinfo.value.code == 2 + assert output.read_bytes() == b'precious' + + +def test_keyboard_interrupt_exits_130( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + file: pathlib.Path = tmp_path / 'data.bin' + file.write_bytes(b'x' * 4096) + + class InterruptingIO(io.BytesIO): + def write(self, data: object) -> int: + raise KeyboardInterrupt + + monkeypatch.setattr( + main, '_get_output_stream', lambda *args: InterruptingIO() + ) + with pytest.raises(SystemExit) as excinfo: + main.main([str(file)]) + assert excinfo.value.code == main.EXIT_INTERRUPTED + + +def test_inputs_are_closed_after_their_copy( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + files: list[pathlib.Path] = [] + for index in range(3): + file: pathlib.Path = tmp_path / f'data{index}.bin' + file.write_bytes(b'x' * 8) + files.append(file) + + opened: list[io.BufferedReader] = [] + open_counts: list[int] = [] + original_open = pathlib.Path.open + + def recording_open(self: pathlib.Path, *args: object, **kwargs: object): + open_counts.append(sum(not stream.closed for stream in opened)) + stream = original_open(self, *args, **kwargs) # type: ignore[arg-type] + opened.append(stream) + return stream + + monkeypatch.setattr(pathlib.Path, 'open', recording_open) + main.main([*map(str, files), '-o', str(tmp_path / 'out.bin')]) + assert open_counts == [0, 0, 0] + assert all(stream.closed for stream in opened) + assert (tmp_path / 'out.bin').read_bytes() == b'x' * 24 + + +def test_output_directory_is_rejected(tmp_path: pathlib.Path) -> None: + source: pathlib.Path = tmp_path / 'in.bin' + source.write_bytes(b'data') + with pytest.raises(SystemExit) as excinfo: + main.main([str(source), '-o', str(tmp_path)]) + assert excinfo.value.code == 2 + + +def test_existing_output_distinct_from_inputs_is_overwritten( + tmp_path: pathlib.Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + source: pathlib.Path = tmp_path / 'in.bin' + source.write_bytes(b'new') + output: pathlib.Path = tmp_path / 'out.bin' + output.write_bytes(b'old contents') + monkeypatch.setattr('sys.stdin', io.TextIOWrapper(io.BytesIO(b''))) + main.main([str(source), '-', '-o', str(output)]) + assert output.read_bytes() == b'new' + + +def test_empty_size_option_means_not_given( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + source: pathlib.Path = tmp_path / 'in.bin' + source.write_bytes(b'data') + output: pathlib.Path = tmp_path / 'out.bin' + monkeypatch.setattr('sys.stdin', io.TextIOWrapper(io.BytesIO(b''))) + main.main( + ['-q', '-s', '', '-B', '', '-L', '', str(source), '-o', str(output)] + ) + assert output.read_bytes() == b'data' + + +def test_output_on_a_device_shared_with_an_input_is_allowed() -> None: + if not pathlib.Path('/dev/null').exists(): + pytest.skip('no /dev/null on this platform') + main.main(['-q', '/dev/null', '-o', '/dev/null']) # must not exit 2 + + +@pytest.mark.parametrize('mode', [[], ['-l']]) +def test_pipeline_reader_closing_early_exits_zero( + tmp_path: pathlib.Path, mode: list[str] +) -> None: + import os + import subprocess + import sys + + if not hasattr(os, 'fork'): + pytest.skip('needs a POSIX pipe') + source: pathlib.Path = tmp_path / 'big.bin' + source.write_bytes(b'line of text\n' * 200_000) + env: dict[str, str] = dict(os.environ) + env['PYTHONPATH'] = str(pathlib.Path(progressbar.__file__).parents[1]) + proc = subprocess.Popen( + [sys.executable, '-m', 'progressbar', '-q', *mode, str(source)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=env, + ) + assert proc.stdout is not None + proc.stdout.read(1) + proc.stdout.close() # the reader goes away, as `| head -c 1` does + _, stderr = proc.communicate(timeout=60) + assert proc.returncode == 0, stderr.decode() + assert b'Exception ignored' not in stderr + + +def test_broken_pipe_on_stdout_discards_the_descriptor( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import io + + file: pathlib.Path = tmp_path / 'data.bin' + file.write_bytes(b'x' * 1024) + + class BrokenStdout(io.BytesIO): + def write(self, data) -> int: + raise BrokenPipeError + + broken: BrokenStdout = BrokenStdout() + discarded: list[bool] = [] + monkeypatch.setattr(main, '_get_output_stream', lambda *args: broken) + monkeypatch.setattr(main.sys, 'stdout', broken) + monkeypatch.setattr( + main, '_discard_broken_stdout', lambda: discarded.append(True) + ) + main.main([str(file)]) + assert discarded == [True] + + +def test_discard_broken_stdout_points_fd_at_devnull( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import os + + read_end, write_end = os.pipe() + os.close(read_end) # the reader is gone + saved_stdout: int = os.dup(1) + os.dup2(write_end, 1) + + class FakeStdout: + def fileno(self) -> int: + return 1 + + monkeypatch.setattr(main.sys, 'stdout', FakeStdout()) + try: + main._discard_broken_stdout() + os.write(1, b'late output') # would raise EPIPE before + assert os.fstat(1).st_ino == os.stat(os.devnull).st_ino + finally: + os.dup2(saved_stdout, 1) + os.close(saved_stdout) + os.close(write_end) + + +def test_discard_broken_stdout_leaves_other_descriptors_alone( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import io + import os + + read_end, write_end = os.pipe() + + class PipeStdout: # an application's own pager pipe installed as stdout + def fileno(self) -> int: + return write_end + + monkeypatch.setattr(main.sys, 'stdout', PipeStdout()) + try: + main._discard_broken_stdout() + os.write(write_end, b'XYZ') + assert os.read(read_end, 3) == b'XYZ' # still the pipe, not devnull + finally: + os.close(read_end) + os.close(write_end) + + monkeypatch.setattr(main.sys, 'stdout', io.StringIO()) + main._discard_broken_stdout() # no fileno(): nothing to do, no raise From 697d5ebf0311afc14296cd8eb958c177176313e1 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 22 Oct 2026 20:21:23 +0200 Subject: [PATCH 05/24] Keep batch labels inert and finish every task bar in the parallel display Batch item labels are data, not terminal markup: every ESC-introduced sequence and every C0/C1 control is stripped before the label becomes a MultiBar key, so an item cannot move the cursor, retitle the terminal or hide output from the other rows. Labels that are not str are rendered through str() first. A finished task's bar is finished (end='') before it is dropped so its stream listeners, capture refcounts and resize handler return to baseline instead of leaking per task. In the shell runner the {} and {item} placeholders are substituted in a single regex pass, so placeholder text inside an item is never expanded again, and shell=True with a list or callable template is a TypeError: subprocess would run only the first element through the shell and drop the item. --- progressbar/_parallel/_display.py | 33 +++++++++-- progressbar/_parallel/_shell.py | 29 +++++++-- tests/test_parallel_display_security.py | 78 +++++++++++++++++++++++++ tests/test_parallel_shell.py | 27 +++++++++ 4 files changed, 159 insertions(+), 8 deletions(-) create mode 100644 tests/test_parallel_display_security.py diff --git a/progressbar/_parallel/_display.py b/progressbar/_parallel/_display.py index d481a31a..20e9ba6f 100644 --- a/progressbar/_parallel/_display.py +++ b/progressbar/_parallel/_display.py @@ -15,6 +15,7 @@ from __future__ import annotations import os +import re import sys import typing @@ -28,6 +29,22 @@ if typing.TYPE_CHECKING: from .. import multi as multi_module +# Batch items are data, not caller-authored markup. Remove every +# ESC-introduced sequence and every C0/C1 control (CR, LF and TAB +# included) so the label is inert text on exactly one terminal row. +_LABEL_CONTROL_RE: re.Pattern[str] = re.compile( + r'\x1b\[[0-?]*[ -/]*[@-~]' # CSI + r'|\x1b\][^\x07\x1b\x9c]*(?:\x07|\x1b\\|\x9c)?' # OSC + r'|\x1b[PX^_][^\x1b\x9c]*(?:\x1b\\|\x9c)?' # DCS, SOS, PM, APC + r'|\x1b[ -/]*[0-~]?' # other ESC sequences + r'|[\x00-\x1f\x7f-\x9f]' # C0, DEL, C1 +) + + +def _display_label(label: str) -> str: + """Return a batch-item label as inert, single-row display text.""" + return _LABEL_CONTROL_RE.sub('', label) + @typing.runtime_checkable class Display(typing.Protocol): @@ -251,7 +268,7 @@ def task_started( self, seq: int, label: str ) -> bar_module.ProgressBar | None: """Add a per-task bar and hand it out for `current_task_bar`.""" - key: str = f'{seq}: {label}' + key: str = f'{seq}: {_display_label(str(label))}' self._keys[seq] = key task_bar: bar_module.ProgressBar = bar_module.ProgressBar( max_value=base.UnknownLength, @@ -261,10 +278,18 @@ def task_started( return task_bar def task_finished(self, seq: int, ok: bool) -> None: - """Drop the task's bar -- finished bars must not pile up.""" + """Drop and finish the task's bar. + + Finished bars must not pile up in the MultiBar, and each bar + must be finished so its stream listeners, capture refcounts and + resize-handler registration return to their baseline. + """ key: str | None = self._keys.pop(seq, None) - if key is not None and key in self.multibar: - del self.multibar[key] + if key is None or key not in self.multibar: + return + task_bar: bar_module.ProgressBar = self.multibar[key] + del self.multibar[key] + task_bar.finish(end='', dirty=not ok) def advance(self, n: int = 1) -> None: """Count completions on the overall bar. diff --git a/progressbar/_parallel/_shell.py b/progressbar/_parallel/_shell.py index bc747788..04ce6ba8 100644 --- a/progressbar/_parallel/_shell.py +++ b/progressbar/_parallel/_shell.py @@ -10,6 +10,7 @@ import functools import os +import re import shlex import subprocess import typing @@ -19,6 +20,10 @@ #: The two placeholder spellings recognized in command templates. _PLACEHOLDERS: tuple[str, str] = ('{}', '{item}') +#: Matches either placeholder spelling, so a template is substituted in +#: one pass and placeholder text inside an item is never re-expanded. +_PLACEHOLDER_RE: re.Pattern[str] = re.compile(r'\{(?:item)?\}') + #: A command template: a string, an argv list, or a callable that #: builds the argv for one item. CommandT = ( @@ -29,10 +34,21 @@ def _substitute(token: str, item_text: str) -> str: - """Replace the placeholder spellings inside one token.""" - for placeholder in _PLACEHOLDERS: - token = token.replace(placeholder, item_text) - return token + """Replace the placeholder spellings inside one token in one pass.""" + return _PLACEHOLDER_RE.sub(lambda _match: item_text, token) + + +def _check_shell(command: CommandT, *, shell: bool) -> None: + """Reject ``shell=True`` with a list or callable template. + + `subprocess.run` with a sequence and ``shell=True`` runs only the + first element through the shell and drops the rest, item included. + """ + if shell and not isinstance(command, str): + raise TypeError( + 'shell=True requires a str command template, got ' + f'{type(command).__name__}' + ) def _has_placeholder(tokens: typing.Iterable[str]) -> bool: @@ -66,7 +82,11 @@ def build_argv( Returns: The argv list, or the command string when ``shell=True``. + + Raises: + TypeError: ``shell=True`` with a list or callable template. """ + _check_shell(command, shell=shell) item_text: str = str(item) if callable(command): return [str(part) for part in command(item)] @@ -120,6 +140,7 @@ def make_runner( env: typing.Any = None, ) -> typing.Callable[[typing.Any], subprocess.CompletedProcess[typing.Any]]: """Bind a command template into a per-item callable for `map`.""" + _check_shell(command, shell=shell) return functools.partial( _run_one, command, diff --git a/tests/test_parallel_display_security.py b/tests/test_parallel_display_security.py new file mode 100644 index 00000000..fd107a59 --- /dev/null +++ b/tests/test_parallel_display_security.py @@ -0,0 +1,78 @@ +"""Security regressions for the ``bar='multi'`` parallel display.""" + +from __future__ import annotations + +import io +import time +import typing + +import pytest + +import progressbar +from progressbar.utils import streams + +_HOSTILE_ITEM: str = 'a\x1b]0;x\x07b\x1bcc\x9b31md\re\nf' +_FORBIDDEN: tuple[str, ...] = ('\x1b]0;x\x07', '\x1bc', '\x9b', '\r', '\n') + + +def _identity(item: typing.Any) -> typing.Any: + return item + + +def _slow_identity(item: typing.Any) -> typing.Any: + # Keep the task running long enough for the render thread to draw it. + time.sleep(0.3) + return item + + +@pytest.mark.no_freezegun +def test_multi_labels_strip_control_sequences() -> None: + fd: io.StringIO = io.StringIO() + result: list[typing.Any] = list( + progressbar.map( + _slow_identity, + [_HOSTILE_ITEM], + bar='multi', + fd=fd, + poll_interval=0.05, + ) + ) + assert result == [_HOSTILE_ITEM] + output: str = fd.getvalue() + for sequence in ('\x1b]0;x\x07', '\x1bc', '\x9b'): + assert sequence not in output + assert '1: ab' in output + # The raw label's CR and LF must not reach the output inside it. + assert 'b\rc' not in output + assert 'd\re' not in output + assert 'e\nf' not in output + + +def test_display_label_removes_every_control() -> None: + from progressbar._parallel import _display + + cleaned: str = _display._display_label(_HOSTILE_ITEM + '\t\x7f\x1b[31m') + for sequence in (*_FORBIDDEN, '\t', '\x7f', '\x1b'): + assert sequence not in cleaned + + +def test_multi_map_restores_stream_state() -> None: + listeners_before: int = len(streams.listeners) + capturing_before: int = streams.capturing + result: list[int] = list( + progressbar.map(_identity, range(5), bar='multi', fd=io.StringIO()) + ) + assert result == list(range(5)) + assert len(streams.listeners) == listeners_before + assert streams.capturing == capturing_before + + +def test_task_started_accepts_a_non_str_label() -> None: + from progressbar._parallel import _display + + display = _display.MultiDisplay( + total=1, poll_interval=0.05, bar_kwargs={'fd': io.StringIO()} + ) + display.task_started(1, 123) # type: ignore[arg-type] + assert display._keys[1] == '1: 123' + display.task_finished(1, ok=True) diff --git a/tests/test_parallel_shell.py b/tests/test_parallel_shell.py index 100f6626..86b272b6 100644 --- a/tests/test_parallel_shell.py +++ b/tests/test_parallel_shell.py @@ -84,8 +84,35 @@ def test_shell_string_appends_quoted(self) -> None: command = _shell.build_argv('gzip -k', 'a file.txt', shell=True) assert command == "gzip -k 'a file.txt'" + def test_item_placeholder_text_is_not_reexpanded(self) -> None: + # One substitution pass: '{item}' inside the item stays literal. + argv: list[str] | str = _shell.build_argv( + 'echo {}', 'a{item}b', shell=False + ) + assert argv == ['echo', 'a{item}b'] + + def test_shell_item_placeholder_text_is_not_reexpanded(self) -> None: + command: list[str] | str = _shell.build_argv( + 'echo {item}', 'x{}y', shell=True + ) + assert command == 'echo x{}y' + + @pytest.mark.parametrize( + 'command', + [['echo', '{}'], lambda item: ['echo', str(item)]], + ) + def test_shell_rejects_non_str_template( + self, command: _shell.CommandT + ) -> None: + with pytest.raises(TypeError, match='str command template'): + _shell.build_argv(command, 'x', shell=True) + class TestRun: + def test_shell_with_list_template_raises(self) -> None: + with pytest.raises(TypeError, match='str command template'): + _shell.run(['echo', '{}'], ['x'], shell=True) + @pytest.mark.no_freezegun def test_runs_commands_and_returns_completed_processes(self) -> None: results: list[subprocess.CompletedProcess[str]] = _shell.run( From a0bdce4f16cd6811c9e3b147cf740d3fdcea05fe Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 22 Oct 2026 20:21:23 +0200 Subject: [PATCH 06/24] Only run the Windows console-mode probe on a tty The ANSI detection called the console-mode probe on Windows for every stream, including pipes and files, where it reports on the console the process happens to own rather than on the stream being asked about. The probe now runs only when the stream is a tty, matching the other platforms. --- progressbar/env.py | 11 ++++++----- tests/test_env_detection.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/progressbar/env.py b/progressbar/env.py index 7c81c8d1..2b7c7dcb 100644 --- a/progressbar/env.py +++ b/progressbar/env.py @@ -162,10 +162,11 @@ def is_ansi_terminal( PyCharm terminal (not under pytest) short-circuits straight to `True`, since both render ANSI without being a tty. Otherwise, `fd.isatty()` plus a `TERM` match against `ANSI_TERM_RE`, or - `ANSICON` being set, or (on Windows) the console-mode probe (see - `os_specific.windows.get_console_mode` for what that probe actually - tests, which is not what its flag name suggests). Only the errors a - stream can legitimately raise while being probed are swallowed: + `ANSICON` being set, or (on Windows, for a tty only) the + console-mode probe (see `os_specific.windows.get_console_mode` for + what that probe actually tests, which is not what its flag name + suggests). Only the errors a stream can legitimately raise while + being probed are swallowed: `OSError` (real I/O), `ValueError` (closed/detached file) and `AttributeError` (no `isatty` at all). Anything else is a bug and propagates. @@ -199,7 +200,7 @@ def is_ansi_terminal( # ANSICON is a Windows ANSI compatible console elif 'ANSICON' in os.environ: is_terminal = True - elif os.name == 'nt': + elif os.name == 'nt' and is_tty: from .terminal.os_specific import windows return bool( diff --git a/tests/test_env_detection.py b/tests/test_env_detection.py index 8a393a0f..dbfb4b32 100644 --- a/tests/test_env_detection.py +++ b/tests/test_env_detection.py @@ -10,6 +10,11 @@ from __future__ import annotations +import enum +import io +import os +import sys +import types import typing import pytest @@ -93,3 +98,32 @@ def test_is_terminal_plain_tty_without_ansi(clean_environment: None) -> None: def test_is_terminal_non_tty(clean_environment: None) -> None: fd = typing.cast(typing.IO[str], TtyFd(False)) assert env.is_terminal(fd) is False + + +def test_windows_console_mode_ignored_for_non_tty( + clean_environment: None, + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The console-mode probe reads the process console, not `fd`: a + # redirected stream must not be reported as an ANSI terminal just + # because the console happens to have processed output enabled. + fake_windows: types.ModuleType = types.ModuleType('fake_windows') + + class FakeFlags(enum.IntFlag): + ENABLE_PROCESSED_OUTPUT = 1 + + def get_console_mode() -> int: # pragma: no cover - must not be called + return 1 + + fake_windows.WindowsConsoleModeFlags = FakeFlags # type: ignore[attr-defined] + fake_windows.get_console_mode = get_console_mode # type: ignore[attr-defined] + monkeypatch.setitem( + sys.modules, 'progressbar.terminal.os_specific.windows', fake_windows + ) + monkeypatch.setattr( + 'progressbar.terminal.os_specific.windows', + fake_windows, + raising=False, + ) + monkeypatch.setattr(os, 'name', 'nt') + assert env.is_ansi_terminal(io.StringIO()) is not True From ce69c60a28f442090eb03676d6738efa138d5d03 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 22 Oct 2026 20:21:23 +0200 Subject: [PATCH 07/24] Guard the release workflow against the wrong ref and stray files The publish job runs only for a v* tag whose name matches __version__ and whose commit is the checked-out commit, so a manual dispatch on a branch or a mistyped tag cannot publish. The built sdist and wheel are listed and refused when they contain sync-conflict, .orig, .rej or .bak files. The workflow token is limited to contents: read. Local secret and environment files are ignored by git. --- .github/workflows/release.yml | 61 ++++++++++++++++++++++++++++++++++- .gitignore | 8 +++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 934fe608..32732927 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -10,6 +10,9 @@ on: required: true type: string +permissions: + contents: read + jobs: build: name: Build distributions @@ -18,15 +21,71 @@ jobs: - uses: actions/checkout@v6 with: ref: ${{ inputs.ref || github.ref }} + - name: Verify the ref is a release tag matching __version__ + env: + EVENT_NAME: ${{ github.event_name }} + EVENT_REF: ${{ github.ref }} + INPUT_REF: ${{ inputs.ref }} + run: | + set -euo pipefail + if [ "$EVENT_NAME" = "workflow_dispatch" ]; then + ref="$INPUT_REF" + else + ref="$EVENT_REF" + fi + case "$ref" in + refs/tags/v*) tag="${ref#refs/tags/}" ;; + v*) + if [ "$EVENT_NAME" != "workflow_dispatch" ]; then + echo "::error::ref '$ref' is not a tag ref" + exit 1 + fi + tag="$ref" + ;; + *) + echo "::error::ref '$ref' is not a v* release tag" + exit 1 + ;; + esac + if ! [[ "$tag" =~ ^v[0-9]+\.[0-9]+\.[0-9]+[0-9A-Za-z.+-]*$ ]]; then + echo "::error::tag '$tag' does not look like vX.Y.Z" + exit 1 + fi + version="$(sed -n "s/^__version__ = ['\"]\([^'\"]*\)['\"].*/\1/p" progressbar/__about__.py)" + if [ -z "$version" ]; then + echo "::error::could not read __version__ from progressbar/__about__.py" + exit 1 + fi + if [ "$tag" != "v$version" ]; then + echo "::error::tag '$tag' does not match __version__ '$version'" + exit 1 + fi + tag_commit="$(git rev-parse --verify --quiet "refs/tags/$tag^{commit}")" || { + echo "::error::tag '$tag' is not present in the checkout" + exit 1 + } + if [ "$tag_commit" != "$(git rev-parse HEAD)" ]; then + echo "::error::checked-out commit is not the commit of tag '$tag'" + exit 1 + fi + echo "Releasing $tag ($tag_commit)" - name: Install uv uses: astral-sh/setup-uv@v7 - name: Build sdist and wheel run: uv build --no-sources - - name: Verify wheel contains only the progressbar package + - name: Verify distribution contents run: | + set -euo pipefail unpacked="$(unzip -Z1 dist/*.whl | cut -d/ -f1 | sort -u)" echo "$unpacked" test "$(echo "$unpacked" | grep -cvE '^progressbar$|^progressbar2-.*\.dist-info$')" -eq 0 + sdist="$(tar tzf dist/*.tar.gz)" + echo "$sdist" + stray_pattern='\.sync-conflict-|\.orig|\.rej|\.bak' + if { unzip -Z1 dist/*.whl; echo "$sdist"; } | grep -E "$stray_pattern"; then + echo "::error::distribution contains stray conflict or backup files" + exit 1 + fi - uses: actions/upload-artifact@v6 with: name: dist diff --git a/.gitignore b/.gitignore index ccdf6182..1e2abf8a 100644 --- a/.gitignore +++ b/.gitignore @@ -48,3 +48,11 @@ docs/_build/ # Built at docs build time for the in-browser console docs/_static/wheels/ + +# Local secrets and environments +.env +.env.* +.venv/ +*.pem +*.key +.pypirc From 06f6efa84220cabf4f35faf47ec9c373ffb73b06 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Sun, 25 Oct 2026 14:02:38 +0100 Subject: [PATCH 08/24] Keep StreamWrapper transitions free of call-outs and fork-safe Two adversarial reviews of the previous commit found that the transition lock was still held while logging ("wrapping excepthook") and while flushing captured text into the original streams. A logging handler that writes into a wrapper, or a target stream with its own lock, could then wait on a thread that was waiting for the transition lock: a deadlock 4.6.0 could not have, because it held no lock at all. Moving the call-outs outside the lock naively made two overlapping stop_capturing() calls deliver the same text twice. Now every transition takes the buffered text under the lock (WrappingIO._take) and writes it to the original stream after releasing it (WrappingIO._deliver), so nothing blocks while the lock is held and no text is delivered twice. The excepthook installation logs after the lock is released. The locks live in a small shared box, _SharedLock, that a StreamWrapper and the wrappers it installs hold in common. The box keeps its identity through pickle and deepcopy whichever object is the root, so restored wrappers share the restored lock, subclass slots survive (default pickling again, no __getstate__), subclasses that define __eq__ without __hash__ construct, and a forked child gets fresh unlocked locks for every live box through os.register_at_fork, standalone wrappers included. Lock annotations evaluate on 3.10 to 3.12, where threading.Lock is a factory function. What working code can notice: StreamWrapper.lock and listeners_lock are read-only properties (both were introduced on this branch, 4.6.0 had neither), and get_type_hints(WrappingIO.__init__) works again on 3.10 to 3.12. --- progressbar/utils.py | 345 ++++++++++++++++++++--------- tests/test_stream_security.py | 394 ++++++++++++++++++++++++++++++++++ 2 files changed, 640 insertions(+), 99 deletions(-) diff --git a/progressbar/utils.py b/progressbar/utils.py index 271c192e..42ce09c3 100644 --- a/progressbar/utils.py +++ b/progressbar/utils.py @@ -20,6 +20,7 @@ import sys import threading import typing +import weakref from collections.abc import Callable, Iterable, Iterator, Mapping from types import TracebackType @@ -171,6 +172,68 @@ def len_color(value: types.StringTypes) -> int: return len(no_color(value)) +#: One installed wrapper's drained text, waiting to be written to the +#: original stream once the transition lock is released. +_Pending = tuple[str, 'WrappingIO', str] + +if typing.TYPE_CHECKING: + _Lock = threading.Lock +else: + #: `threading.Lock` is a factory function before Python 3.13, so an + #: annotation that names it does not evaluate there + #: (`typing.get_type_hints` raises). The instance type does. + _Lock = type(threading.Lock()) + + +class _SharedLock: + """A lock that several objects share, and that survives pickle and fork. + + `StreamWrapper` and every `WrappingIO` it installs hold the same + box, so they agree on one lock however the graph is pickled or + deep-copied (the box is one object, restored once, whichever object + is the root). The lock itself is never pickled: a restored box gets + a fresh one. Every live box is registered so a forked child can + replace the lock it inherited, which another parent thread may + have been holding at fork time and would otherwise stay locked + forever in the child. + """ + + __slots__ = ('__weakref__', '_factory', 'lock') + + def __init__( + self, + factory: Callable[[], typing.Any] = threading.Lock, + lock: typing.Any = None, + ) -> None: + self._factory: Callable[[], typing.Any] = factory + self.lock: typing.Any = lock if lock is not None else factory() + _lock_boxes.add(self) + + def reset(self) -> None: + """Replace the lock with a new, unlocked one (after a fork).""" + self.lock = self._factory() + + def __getstate__(self) -> Callable[[], typing.Any]: + return self._factory + + def __setstate__(self, factory: Callable[[], typing.Any]) -> None: + self._factory = factory + self.lock = factory() + _lock_boxes.add(self) + + +_lock_boxes: weakref.WeakSet[_SharedLock] = weakref.WeakSet() + + +def _reset_locks_after_fork() -> None: # pragma: no cover + for box in list(_lock_boxes): + box.reset() + + +if hasattr(os, 'register_at_fork'): # pragma: no branch + os.register_at_fork(after_in_child=_reset_locks_after_fork) + + class WrappingIO: """`sys.stdout`/`sys.stderr` replacement installed while capturing. @@ -184,15 +247,27 @@ class WrappingIO: target: base.IO capturing: bool listeners: set[_ProgressListener] - lock: threading.Lock + #: Holds the lock that guards `listeners` only. The class-level + #: default keeps a subclass that skips `super().__init__()` working, + #: as for `StreamWrapper`. + _lock_box: _SharedLock = _SharedLock() needs_clear: bool = False + @property + def lock(self) -> _Lock: + """The lock guarding `listeners`, shared with the installer.""" + return self._lock_box.lock + + @lock.setter + def lock(self, lock: _Lock) -> None: + self._lock_box = _SharedLock(threading.Lock, lock) + def __init__( self, target: base.IO, capturing: bool = False, listeners: set[_ProgressListener] | None = None, - lock: threading.Lock | None = None, + lock: _Lock | _SharedLock | None = None, ) -> None: """Wrap `target` so writes can be buffered while `capturing`. @@ -218,20 +293,12 @@ def __init__( self.target = target self.capturing = capturing self.listeners = listeners if listeners is not None else set() - self.lock = lock if lock is not None else threading.Lock() + if isinstance(lock, _SharedLock): + self._lock_box = lock + else: + self._lock_box = _SharedLock(threading.Lock, lock) self.needs_clear = False - def __getstate__(self) -> dict[str, typing.Any]: - """Pickle everything but the lock, which is recreated on load.""" - state: dict[str, typing.Any] = self.__dict__.copy() - state.pop('lock', None) - return state - - def __setstate__(self, state: dict[str, typing.Any]) -> None: - """Restore the state with a fresh, private lock.""" - self.__dict__.update(state) - self.lock = threading.Lock() - def write(self, value: str) -> int: """Write `value`, buffering it in memory while `capturing`. @@ -286,16 +353,29 @@ def _flush(self) -> None: end, even when the buffer was empty, since this runs on every bar redraw, not just when there's something to flush. """ - if value := self.buffer.getvalue(): + self._deliver(self._take()) + + def _take(self) -> str: + """Drain and return the buffered text without touching `target`. + + `StreamWrapper` calls this under its transition lock, so two + overlapping flushes cannot both pick up the same text, and + delivers the result after releasing it. + """ + value: str = self.buffer.getvalue() + if value: self.flush() # Clear the buffer before writing so a failed write cannot # cause the same data to be written again by the next flush self.buffer.seek(0) self.buffer.truncate(0) self.needs_clear = False - if not self.target.closed: - self.target.write(value) + return value + def _deliver(self, value: str) -> None: + """Write text taken by `_take()` to `target`, then flush it.""" + if value and not self.target.closed: + self.target.write(value) # when explicitly flushing, always flush the target as well self.flush_target() @@ -406,29 +486,28 @@ class StreamWrapper: logging_handlers: list[tuple[logging.StreamHandler[base.IO], base.IO]] capturing: int = 0 listeners: set[_ProgressListener] - #: Guards every wrap/unwrap/capture transition. Reentrant, because - #: wrap_stdout() calls wrap_excepthook() and update_capturing() calls - #: flush() while holding it. The class-level default keeps a subclass - #: that skips `super().__init__()` working, as every counter above. - lock: threading.RLock = threading.RLock() - #: Guards `listeners` only. Shared with every `WrappingIO` this - #: wrapper installs, and never held while calling out, so a captured - #: `write()` on one thread cannot wait for `lock` (which is held - #: across flushes into the original stream, logging included). - listeners_lock: threading.Lock = threading.Lock() - - def __getstate__(self) -> dict[str, typing.Any]: - """Pickle everything but the locks, which are recreated on load.""" - state: dict[str, typing.Any] = self.__dict__.copy() - state.pop('lock', None) - state.pop('listeners_lock', None) - return state - - def __setstate__(self, state: dict[str, typing.Any]) -> None: - """Restore the state with fresh, unlocked locks.""" - self.__dict__.update(state) - self.lock = threading.RLock() - self.listeners_lock = threading.Lock() + #: Holds the lock guarding every wrap/unwrap/capture transition. + #: Reentrant, because wrap_stdout() calls wrap_excepthook() while + #: holding it. Nothing is logged and no original stream is written + #: while it is held, so it cannot take part in a lock-order cycle + #: with a logging handler or a locking target stream. The + #: class-level default keeps a subclass that skips + #: `super().__init__()` working, as every counter above. + _lock_box: _SharedLock = _SharedLock(threading.RLock) + #: Holds the lock guarding `listeners` only. Shared with every + #: `WrappingIO` this wrapper installs, and never held while calling + #: out. + _listeners_box: _SharedLock = _SharedLock() + + @property + def lock(self) -> threading.RLock: + """The reentrant lock guarding wrap/unwrap/capture transitions.""" + return self._lock_box.lock + + @property + def listeners_lock(self) -> _Lock: + """The lock guarding `listeners`, shared with the wrappers.""" + return self._listeners_box.lock def __init__(self) -> None: """Capture the *current* `sys.stdout`/`sys.stderr` as "real". @@ -452,8 +531,8 @@ def __init__(self) -> None: self.logging_handlers = [] self.capturing = 0 self.listeners = set() - self.lock = threading.RLock() - self.listeners_lock = threading.Lock() + self._lock_box = _SharedLock(threading.RLock) + self._listeners_box = _SharedLock() if env.env_flag('WRAP_STDOUT', default=False): # pragma: no cover self.wrap_stdout() @@ -475,7 +554,8 @@ def start_capturing(self, bar: _ProgressListener | None = None) -> None: self.listeners.add(bar) self.capturing += 1 - self.update_capturing() + # Never flushes here: the count just went above zero + self._apply_capturing() def stop_capturing(self, bar: _ProgressListener | None = None) -> None: """Unregister `bar` and drop the shared capturing refcount. @@ -492,7 +572,8 @@ def stop_capturing(self, bar: _ProgressListener | None = None) -> None: # Never go negative: an unmatched stop would otherwise make # the next start_capturing() a no-op self.capturing = max(self.capturing - 1, 0) - self.update_capturing() + pending: list[_Pending] = self._apply_capturing() + self._deliver(pending) def update_capturing(self) -> None: # pragma: no cover """Propagate the capturing refcount to the wrapped streams. @@ -503,14 +584,65 @@ def update_capturing(self) -> None: # pragma: no cover until something else happens to trigger a flush. """ with self.lock: - if isinstance(self.stdout, WrappingIO): - self.stdout.capturing = self.capturing > 0 + pending: list[_Pending] = self._apply_capturing() + self._deliver(pending) + + def _apply_capturing(self) -> list[_Pending]: + """Push the capture state onto the wrappers, under `lock`. + + Returns the buffered text taken from each installed wrapper + when the refcount reached zero, for the caller to deliver after + releasing `lock`: a delivery writes into the original streams, + and one of those may be a logging handler or a widget that + takes a lock of its own on another thread. Taking the text + under the lock means two overlapping flushes never deliver the + same text twice, and an unwrap cannot swap the wrapper out + between the type check and the drain. + """ + if isinstance(self.stdout, WrappingIO): + self.stdout.capturing = self.capturing > 0 + + if isinstance(self.stderr, WrappingIO): + self.stderr.capturing = self.capturing > 0 - if isinstance(self.stderr, WrappingIO): - self.stderr.capturing = self.capturing > 0 + if self.capturing > 0: + return [] + return self._take() - if self.capturing <= 0: - self.flush() + def _take(self) -> list[_Pending]: + """Drain both installed wrappers' buffers, under `lock`.""" + pending: list[_Pending] = [] + if self.wrapped_stdout and isinstance(self.stdout, WrappingIO): + pending.append(('stdout', self.stdout, self.stdout._take())) + if self.wrapped_stderr and isinstance(self.stderr, WrappingIO): + pending.append(('stderr', self.stderr, self.stderr._take())) + return pending + + def _deliver(self, pending: list[_Pending]) -> None: + """Write taken text to the original streams, outside `lock`. + + If writing to a stream's target raises `io.UnsupportedOperation` + (as happens for some non-seekable streams), that stream's + redirection disables itself: `wrapped_stdout`/`wrapped_stderr` + is reset to 0 so no further flush attempts are made, and a + warning is logged. `sys.stdout`/`sys.stderr` are left installed + as-is: only further flush attempts are skipped, not the wrapping + itself. + """ + for name, wrapper, text in pending: + self._deliver_one(name, wrapper, text) + + def _deliver_one(self, name: str, wrapper: WrappingIO, text: str) -> None: + """Deliver one wrapper's text, disabling its redirection on failure.""" + try: + wrapper._deliver(text) + except io.UnsupportedOperation: + setattr(self, f'wrapped_{name}', 0) + logger.warning( + 'Disabling %s redirection, %r is not seekable', + name, + getattr(sys, name), + ) def wrap(self, stdout: bool = False, stderr: bool = False) -> None: """Wrap `stdout` and/or `stderr`, per the given flags.""" @@ -535,17 +667,19 @@ def wrap_stdout(self) -> WrappingIO: call until it's fully unwrapped. """ with self.lock: - self.wrap_excepthook() + hooked: bool = self._wrap_excepthook_locked() if not self.wrapped_stdout: self.stdout = sys.stdout = WrappingIO( # type: ignore self.original_stdout, listeners=self.listeners, - lock=self.listeners_lock, + lock=self._listeners_box, ) self.wrapped_stdout += 1 - - return sys.stdout # type: ignore + wrapper: WrappingIO = sys.stdout # type: ignore + if hooked: + self._log_excepthook_wrapped() + return wrapper def wrap_stderr(self) -> WrappingIO: """Install a `WrappingIO` over `sys.stderr`, or share it. @@ -558,17 +692,19 @@ def wrap_stderr(self) -> WrappingIO: call until it's fully unwrapped. """ with self.lock: - self.wrap_excepthook() + hooked: bool = self._wrap_excepthook_locked() if not self.wrapped_stderr: self.stderr = sys.stderr = WrappingIO( # type: ignore self.original_stderr, listeners=self.listeners, - lock=self.listeners_lock, + lock=self._listeners_box, ) self.wrapped_stderr += 1 - - return sys.stderr # type: ignore + wrapper: WrappingIO = sys.stderr # type: ignore + if hooked: + self._log_excepthook_wrapped() + return wrapper def wrap_logging(self) -> None: """Retarget every `StreamHandler` in the logger tree to the wrapper. @@ -710,10 +846,27 @@ def wrap_excepthook(self) -> None: wrapped it. """ with self.lock: - if not self.wrapped_excepthook: - logger.debug('wrapping excepthook') - self.wrapped_excepthook = True - sys.excepthook = self.excepthook + hooked: bool = self._wrap_excepthook_locked() + if hooked: + self._log_excepthook_wrapped() + + def _wrap_excepthook_locked(self) -> bool: + """Install the excepthook under `lock`; True when newly installed.""" + if self.wrapped_excepthook: + return False + self.wrapped_excepthook = True + sys.excepthook = self.excepthook + return True + + @staticmethod + def _log_excepthook_wrapped() -> None: + """Log the hook installation, after `lock` has been released. + + Logging while holding `lock` could wait for a handler lock held + by a thread that is writing into a wrapper and waiting for + `lock` in turn. + """ + logger.debug('wrapping excepthook') def unwrap(self, stdout: bool = False, stderr: bool = False) -> None: """Unwrap `stdout` and/or `stderr`, per the given flags.""" @@ -724,8 +877,20 @@ def unwrap(self, stdout: bool = False, stderr: bool = False) -> None: self.unwrap_stderr() @staticmethod - def _deliver_dropped(dropped: WrappingIO) -> None: - """Stop buffering in a dropped wrapper and deliver what it holds. + def _drop(dropped: object) -> tuple[WrappingIO, str] | None: + """Stop buffering in a dropped wrapper and take what it holds. + + Runs under `lock`; the text goes to `_deliver_dropped` after + the lock is released. Returns None for a plain stream. + """ + if not isinstance(dropped, WrappingIO): + return None + dropped.capturing = False + return dropped, dropped._take() + + @staticmethod + def _deliver_dropped(taken: tuple[WrappingIO, str] | None) -> None: + """Deliver the text `_drop` took, outside `lock`. A stale reference to the wrapper must not swallow output. The delivery touches the original stream, so a stream that cannot @@ -733,9 +898,11 @@ def _deliver_dropped(dropped: WrappingIO) -> None: is gone) is logged and otherwise ignored: unwrapping never raises for it, as it did not before the flush was added. """ - dropped.capturing = False + if taken is None: + return + dropped, text = taken try: - dropped._flush() + dropped._deliver(text) except Exception: logger.debug( 'Could not deliver buffered output on unwrap', @@ -757,15 +924,16 @@ def unwrap_stdout(self) -> None: self.wrapped_stdout -= 1 return - dropped: object = self.stdout + taken = self._drop(self.stdout) # Also reset our own reference so needs_clear() and # update_capturing() don't act on a stale wrapper self.stdout = sys.stdout = self.original_stdout self.wrapped_stdout = 0 - if isinstance(dropped, WrappingIO): - self._deliver_dropped(dropped) if not self.wrapped_stderr: self.unwrap_excepthook() + # Deliver after releasing `lock`: this writes into the original + # stream and may log, see `_apply_capturing` + self._deliver_dropped(taken) def unwrap_stderr(self) -> None: """Undo one `wrap_stderr()` call, restoring at refcount zero. @@ -780,15 +948,15 @@ def unwrap_stderr(self) -> None: self.wrapped_stderr -= 1 return - dropped: object = self.stderr + taken = self._drop(self.stderr) # Also reset our own reference so needs_clear() and # update_capturing() don't act on a stale wrapper self.stderr = sys.stderr = self.original_stderr self.wrapped_stderr = 0 - if isinstance(dropped, WrappingIO): - self._deliver_dropped(dropped) if not self.wrapped_stdout: self.unwrap_excepthook() + # Deliver after releasing `lock`, see `unwrap_stdout` + self._deliver_dropped(taken) def needs_clear(self) -> bool: # pragma: no cover """Return whether either wrapped stream has buffered output. @@ -808,34 +976,13 @@ def needs_clear(self) -> bool: # pragma: no cover def flush(self) -> None: """Flush buffered captured output on both wrapped streams. - If writing the buffered text to a stream's target raises - `io.UnsupportedOperation` (as happens for some non-seekable - streams), that stream's redirection disables itself: - `wrapped_stdout`/`wrapped_stderr` is reset to 0 so this method - stops attempting to flush it on future calls, and a warning is - logged. `sys.stdout`/`sys.stderr` are left installed as-is: - only further flush attempts are skipped, not the wrapping - itself. + The buffers are drained under `lock` and written to the + original streams after releasing it, see `_deliver` for what + happens when a stream cannot take the text. """ - if self.wrapped_stdout and isinstance(self.stdout, WrappingIO): - try: - self.stdout._flush() - except io.UnsupportedOperation: - self.wrapped_stdout = 0 - logger.warning( - 'Disabling stdout redirection, %r is not seekable', - sys.stdout, - ) - - if self.wrapped_stderr and isinstance(self.stderr, WrappingIO): - try: - self.stderr._flush() - except io.UnsupportedOperation: # pragma: no cover - self.wrapped_stderr = 0 - logger.warning( - 'Disabling stderr redirection, %r is not seekable', - sys.stderr, - ) + with self.lock: + pending: list[_Pending] = self._take() + self._deliver(pending) def excepthook( self, diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py index 4d44e290..de0f8c9f 100644 --- a/tests/test_stream_security.py +++ b/tests/test_stream_security.py @@ -1,7 +1,10 @@ from __future__ import annotations import io +import logging import sys +import threading +import typing from collections.abc import Iterator import pytest @@ -272,3 +275,394 @@ def flush(self) -> None: streams.wrap_stdout() streams.unwrap_stdout() # must not raise assert sys.stdout is target + + +# ---- second review round (Codex findings 1-4) --------------------------- + + +class _LockProbeHandler(logging.Handler): + """Record, per log record, whether `lock` was free at emit time.""" + + def __init__(self, lock: threading.RLock) -> None: + super().__init__(level=logging.DEBUG) + self.watched = lock # not `self.lock`: that is the handler's own + self.free: list[bool] = [] + + def emit(self, record: logging.LogRecord) -> None: + result: list[bool] = [] + + def probe() -> None: + got: bool = self.watched.acquire(timeout=1.0) + if got: + self.watched.release() + result.append(got) + + thread: threading.Thread = threading.Thread(target=probe) + thread.start() + thread.join() + self.free.append(result[0]) + + +def test_logging_never_happens_under_the_transition_lock( + monkeypatch: pytest.MonkeyPatch, +) -> None: + class RaisingFlush(io.StringIO): + armed: bool = False + + def flush(self) -> None: + if self.armed: + raise RuntimeError('gone') + + streams: utils.StreamWrapper = utils.StreamWrapper() + original: RaisingFlush = RaisingFlush() + streams.original_stdout = original + streams.stdout = original + monkeypatch.setattr(sys, 'stdout', original) + handler: _LockProbeHandler = _LockProbeHandler(streams.lock) + logger: logging.Logger = logging.getLogger('progressbar.utils') + logger.addHandler(handler) + old_level: int = logger.level + logger.setLevel(logging.DEBUG) + try: + streams.wrap_stdout() # logs 'wrapping excepthook' + streams.start_capturing() + print('buffered') + original.armed = True + streams.unwrap_stdout() # logs 'Could not deliver ...' + streams.stop_capturing() + finally: + logger.removeHandler(handler) + logger.setLevel(old_level) + assert len(handler.free) >= 2 + assert all(handler.free), handler.free + + +def test_targets_are_never_written_under_the_transition_lock( + monkeypatch: pytest.MonkeyPatch, +) -> None: + streams: utils.StreamWrapper = utils.StreamWrapper() + seen: list[bool] = [] + + class LockProbeTarget(io.StringIO): + def write(self, text: str) -> int: + result: list[bool] = [] + + def probe() -> None: + got: bool = streams.lock.acquire(timeout=1.0) + if got: + streams.lock.release() + result.append(got) + + thread: threading.Thread = threading.Thread(target=probe) + thread.start() + thread.join() + seen.append(result[0]) + return super().write(text) + + target: LockProbeTarget = LockProbeTarget() + streams.original_stdout = target + streams.stdout = target + monkeypatch.setattr(sys, 'stdout', target) + streams.wrap_stdout() + streams.start_capturing() + print('one') + streams.stop_capturing() # delivers 'one' to the target + streams.start_capturing() + print('two') + streams.unwrap_stdout() # delivers 'two' to the target + assert target.getvalue() == 'one\ntwo\n' + assert len(seen) >= 2 + assert all(seen), seen + + +class NamedIO(utils.WrappingIO): # pickle needs a module-level class + __slots__ = ('name',) + + def __init__(self, target: io.StringIO, name: str) -> None: + super().__init__(target) + self.name = name + + +class NamedStreams(utils.StreamWrapper): + __slots__ = ('name',) + + def __init__(self, name: str) -> None: + super().__init__() + self.name = name + + +def test_pickle_and_deepcopy_keep_subclass_slots() -> None: + import copy + import pickle + + named_io: NamedIO = NamedIO(io.StringIO(), 'spam-stream') + named_streams: NamedStreams = NamedStreams('spam-streams') + for name in ('stdout', 'stderr'): + setattr(named_streams, f'original_{name}', io.StringIO()) + setattr(named_streams, name, io.StringIO()) + # a builtin pickles by reference only while it is still installed, + # and another test may have left the shared hook wrapped + named_streams.original_excepthook = sys.excepthook + for restore in (copy.deepcopy, lambda o: pickle.loads(pickle.dumps(o))): + assert restore(named_io).name == 'spam-stream' + assert restore(named_streams).name == 'spam-streams' + + +def test_restored_wrappers_share_the_restored_listener_lock( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import copy + import pickle + + streams: utils.StreamWrapper = utils.StreamWrapper() + for name in ('stdout', 'stderr'): + target: io.StringIO = io.StringIO() + setattr(streams, f'original_{name}', target) + setattr(streams, name, target) + monkeypatch.setattr(sys, name, target) + streams.wrap_stdout() + streams.wrap_stderr() + streams.original_excepthook = sys.excepthook # see the slots test + try: + for restored in ( + copy.deepcopy(streams), + pickle.loads(pickle.dumps(streams)), + ): + assert isinstance(restored.stdout, utils.WrappingIO) + assert isinstance(restored.stderr, utils.WrappingIO) + assert restored.stdout.listeners is restored.listeners + assert restored.stdout.lock is restored.listeners_lock + assert restored.stderr.lock is restored.listeners_lock + assert restored.listeners_lock is not streams.listeners_lock + finally: + streams.unwrap_stdout() + streams.unwrap_stderr() + + +def test_wrapping_io_subclass_without_super_init_can_write() -> None: + class NoSuper(utils.WrappingIO): + def __init__(self, target: io.StringIO) -> None: + self.buffer = io.StringIO() + self.target = target + self.capturing = True + self.listeners = set() + self.needs_clear = False + + wrapper: NoSuper = NoSuper(io.StringIO()) + assert wrapper.write('spam\n') == 5 + assert wrapper.needs_clear + + +def test_stream_class_type_hints_evaluate_on_this_python() -> None: + import typing + + # `threading.Lock` is a factory function before 3.13, so an annotation + # naming it directly cannot be evaluated there (regression caught by + # the second hunt). + typing.get_type_hints(utils.WrappingIO.__init__) + typing.get_type_hints(utils.WrappingIO) + typing.get_type_hints(utils.StreamWrapper.__init__) + + +def test_forked_child_gets_fresh_stream_locks() -> None: + import os + + if not hasattr(os, 'fork') or not hasattr(os, 'register_at_fork'): + pytest.skip('needs a POSIX fork') + streams: utils.StreamWrapper = progressbar.streams + held: threading.Event = threading.Event() + release: threading.Event = threading.Event() + + def holder() -> None: + with streams.lock, streams.listeners_lock: + held.set() + release.wait(10) + + thread: threading.Thread = threading.Thread(target=holder, daemon=True) + thread.start() + held.wait(5) + pid: int = os.fork() + if pid == 0: # pragma: no cover - child + status: int = 0 + if not streams.lock.acquire(timeout=2): + status |= 1 + if not streams.listeners_lock.acquire(timeout=2): + status |= 2 + os._exit(status) + try: + _, raw_status = os.waitpid(pid, 0) + finally: + release.set() + thread.join() + assert os.waitstatus_to_exitcode(raw_status) == 0 + + +def test_wrap_excepthook_directly(monkeypatch: pytest.MonkeyPatch) -> None: + streams: utils.StreamWrapper = utils.StreamWrapper() + original: object = sys.excepthook + monkeypatch.setattr(sys, 'excepthook', original) + streams.original_excepthook = original # type: ignore[assignment] + streams.wrap_excepthook() + assert sys.excepthook == streams.excepthook + streams.wrap_excepthook() # idempotent + streams.unwrap_excepthook() + assert sys.excepthook is original + + +# ---- third review round (Codex re-review findings 1-4) ------------------- + + +def test_overlapping_stop_capturing_delivers_buffered_text_once( + monkeypatch: pytest.MonkeyPatch, +) -> None: + paused: threading.Event = threading.Event() + resume: threading.Event = threading.Event() + + class PausingBuffer(io.StringIO): + # The first stop pauses between reading and clearing the + # buffer; a second lifecycle must not drain the same text again + def getvalue(self) -> str: + value: str = super().getvalue() + if threading.current_thread().name == 'first-stop': + paused.set() + assert resume.wait(3) + return value + + streams: utils.StreamWrapper = utils.StreamWrapper() + target: io.StringIO = io.StringIO() + streams.stdout = streams.original_stdout = target + monkeypatch.setattr(sys, 'stdout', target) + wrapper: utils.WrappingIO = streams.wrap_stdout() + streams.start_capturing() + wrapper.buffer = PausingBuffer() + wrapper.write('spam') + first: threading.Thread = threading.Thread( + target=streams.stop_capturing, name='first-stop' + ) + first.start() + assert paused.wait(2) + second_done: threading.Event = threading.Event() + + def another_lifecycle() -> None: + streams.start_capturing() + streams.stop_capturing() + second_done.set() + + second: threading.Thread = threading.Thread(target=another_lifecycle) + second.start() + second_done.wait(0.3) + resume.set() + first.join() + second.join() + streams.unwrap_stdout() + assert target.getvalue() == 'spam' + + +def test_subclass_defining_eq_without_hash_constructs() -> None: + class EqualStreams(utils.StreamWrapper): + def __eq__(self, other: object) -> bool: + return self is other + + streams: EqualStreams = EqualStreams() + assert streams.lock is not utils.StreamWrapper.lock + + +class _BackReference: # a listener that refers to its StreamWrapper + def __init__(self, streams: utils.StreamWrapper) -> None: + self.streams = streams + + def update(self) -> None: + pass + + +def test_cyclic_graph_restores_from_any_root( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import copy + import pickle + + streams: utils.StreamWrapper = utils.StreamWrapper() + for name in ('stdout', 'stderr'): + target: io.StringIO = io.StringIO() + setattr(streams, f'original_{name}', target) + setattr(streams, name, target) + monkeypatch.setattr(sys, name, target) + streams.original_excepthook = sys.excepthook + listener: _BackReference = _BackReference(streams) + wrapper: utils.WrappingIO = streams.wrap_stdout() + streams.start_capturing(listener) # type: ignore[arg-type] + try: + for root in (streams, wrapper, listener): + for restore in ( + copy.deepcopy, + lambda o: pickle.loads(pickle.dumps(o)), + ): + clone: object = restore(root) + if isinstance(clone, utils.WrappingIO): + owner = next(iter(clone.listeners)).streams # type: ignore[attr-defined] + assert clone.lock is owner.listeners_lock + elif isinstance(clone, _BackReference): + owner = clone.streams + else: + owner = clone + assert isinstance(owner.stdout, utils.WrappingIO) + assert owner.stdout.lock is owner.listeners_lock + assert owner.listeners_lock is not streams.listeners_lock + finally: + streams.stop_capturing(listener) # type: ignore[arg-type] + streams.unwrap_stdout() + + +def _fork_and_acquire(current_lock: typing.Callable[[], typing.Any]) -> None: + # `current_lock` is re-read in the child: the fork hook replaces the + # lock object, so a reference taken before the fork stays locked + import os + + if not hasattr(os, 'fork'): + pytest.skip('needs a POSIX fork') + held: threading.Event = threading.Event() + release: threading.Event = threading.Event() + + def holder() -> None: + with current_lock(): + held.set() + release.wait(10) + + thread: threading.Thread = threading.Thread(target=holder, daemon=True) + thread.start() + held.wait(5) + pid: int = os.fork() + if pid == 0: # pragma: no cover - child + os._exit(0 if current_lock().acquire(timeout=2) else 1) + try: + _, raw_status = os.waitpid(pid, 0) + finally: + release.set() + thread.join() + assert os.waitstatus_to_exitcode(raw_status) == 0 + + +def test_forked_child_gets_a_fresh_standalone_wrapper_lock() -> None: + wrapper: utils.WrappingIO = utils.WrappingIO(io.StringIO()) + _fork_and_acquire(lambda: wrapper.lock) + + +def test_forked_child_gets_a_fresh_interval_warning_lock() -> None: + from progressbar import bar as bar_module + + _fork_and_acquire(lambda: bar_module._bad_interval_lock.lock) + + +def test_shared_lock_box_reset_and_wrapper_lock_setter() -> None: + box: utils._SharedLock = utils._SharedLock() + old_lock: object = box.lock + with box.lock: + box.reset() # what the fork hook does: a new, unlocked lock + assert box.lock is not old_lock + assert box.lock.acquire(blocking=False) + box.lock.release() + + wrapper: utils.WrappingIO = utils.WrappingIO(io.StringIO()) + private: threading.Lock = threading.Lock() + wrapper.lock = private # the 4.6.0-era way of handing in a lock + assert wrapper.lock is private From f09451b9d8b21aa584a40ac2b7ce35c5c956956d Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Sun, 25 Oct 2026 14:02:38 +0100 Subject: [PATCH 09/24] Keep the desc prefix a FormatLabel and refuse frame attribute walks ProgressBar(desc=...) rendered the literal text by inserting the plain str into the widget list, so bar.widgets[0] was a str where 4.6.0 (and prefix=) had a FormatLabel; a subclass that rewrites that label after start() silently missed it. The desc-derived prefix is now escaped and wrapped in a FormatLabel when the widgets are built, while bar.prefix keeps the raw f'{desc}: ' value that the fast path renders verbatim. The safe formatter also refuses the non-dunder attributes that lead from a generator, coroutine, traceback or frame stored as a bar variable to a frame's globals (f_globals, f_locals, f_builtins, f_back, gi_frame, cr_frame, ag_frame, tb_frame). The once-per-value warning for a bad PROGRESSBAR_MINIMUM_UPDATE_INTERVAL records the value under a lock before logging, so concurrent first constructions produce one record, remembers every distinct value, and its lock is reset in a forked child. --- progressbar/bar.py | 41 +++++++++++++---- progressbar/widgets.py | 20 +++++++- tests/test_format_security.py | 87 ++++++++++++++++++++++++++++++++++- 3 files changed, 136 insertions(+), 12 deletions(-) diff --git a/progressbar/bar.py b/progressbar/bar.py index 46e61ada..e92e00c4 100644 --- a/progressbar/bar.py +++ b/progressbar/bar.py @@ -76,9 +76,29 @@ def _caller_stacklevel() -> int: _FastBarIterator = None -@functools.lru_cache(maxsize=16) +_bad_interval_seen: set[str] = set() +#: A shared lock box, so a forked child gets an unlocked lock +_bad_interval_lock: utils._SharedLock = utils._SharedLock() + + +def _reset_bad_interval_warnings() -> None: + """Forget the bad values warned about so far (for tests).""" + with _bad_interval_lock.lock: + _bad_interval_seen.clear() + + def _warn_bad_minimum_update_interval(raw: str) -> None: - """Warn once per distinct bad value, not once per bar.""" + """Warn once per distinct bad value, not once per bar. + + The value is recorded under a lock before the warning goes out, so + two bars constructed at the same moment on different threads + produce one record, and the warning itself is logged outside the + lock. Every distinct bad value seen in the process is remembered. + """ + with _bad_interval_lock.lock: + if raw in _bad_interval_seen: + return + _bad_interval_seen.add(raw) logger.warning( 'Ignoring PROGRESSBAR_MINIMUM_UPDATE_INTERVAL=%r: ' 'not a non-negative finite number', @@ -903,12 +923,14 @@ def __init__( if max_value is None and total is not None: # tqdm-style alias for `max_value` max_value = total + self._literal_prefix: str | None = None if prefix is None and desc is not None: # tqdm-style alias for `prefix`: `desc` is literal text, as in # tqdm and as the fast path renders it. Remember the exact - # string, so a prefix assigned later is a template again. + # string, so `_init_prefix` escapes its braces and a prefix + # assigned later is a template again. prefix = f'{desc}: ' - self._literal_prefix: str | None = prefix + self._literal_prefix = prefix if max_value and min_value > typing.cast(NumberT, max_value): raise ValueError( @@ -1722,12 +1744,13 @@ def _init_prefix(self) -> None: if self.prefix: widgets = _load_widgets() - label: typing.Any = ( - self.prefix - if self.prefix is getattr(self, '_literal_prefix', None) - else widgets.FormatLabel(self.prefix, new_style=True) + template: str = self.prefix + if template is getattr(self, '_literal_prefix', None): + # A desc is literal text: escape its braces for the label + template = template.replace('{', '{{').replace('}', '}}') + self.widgets.insert( + 0, widgets.FormatLabel(template, new_style=True) ) - self.widgets.insert(0, label) self.prefix = None def _verify_max_value(self) -> None: diff --git a/progressbar/widgets.py b/progressbar/widgets.py index 8d5e2b58..5fa1a4e9 100644 --- a/progressbar/widgets.py +++ b/progressbar/widgets.py @@ -43,6 +43,22 @@ class _SafeFormatter(string.Formatter): user's business. """ + #: Non-dunder attributes that also lead from an object stored as a + #: bar variable (a generator, coroutine, traceback or frame) to a + #: frame and its globals. + blocked_attributes: frozenset[str] = frozenset( + { + 'f_back', + 'f_builtins', + 'f_globals', + 'f_locals', + 'gi_frame', + 'cr_frame', + 'ag_frame', + 'tb_frame', + } + ) + def get_field( self, field_name: str, @@ -52,7 +68,9 @@ def get_field( rest: typing.Iterable[tuple[bool, typing.Any]] _first, rest = _string.formatter_field_name_split(field_name) for is_attr, name in rest: - if is_attr and str(name).startswith('__'): + if is_attr and ( + str(name).startswith('__') or name in self.blocked_attributes + ): raise ValueError( f'attribute {name!r} is not allowed in a format template', ) diff --git a/tests/test_format_security.py b/tests/test_format_security.py index 27bca3ff..de05ccfa 100644 --- a/tests/test_format_security.py +++ b/tests/test_format_security.py @@ -167,7 +167,7 @@ def test_bad_minimum_update_interval_env_logs_a_warning( from progressbar import bar as bar_module - bar_module._warn_bad_minimum_update_interval.cache_clear() + bar_module._reset_bad_interval_warnings() monkeypatch.setenv('PROGRESSBAR_MINIMUM_UPDATE_INTERVAL', 'spam') with caplog.at_level(logging.WARNING, logger='progressbar.bar'): progressbar.ProgressBar(fd=io.StringIO(), max_value=10) @@ -197,10 +197,93 @@ def test_bad_minimum_update_interval_env_warns_once_per_value( from progressbar import bar as bar_module - bar_module._warn_bad_minimum_update_interval.cache_clear() + bar_module._reset_bad_interval_warnings() monkeypatch.setenv('PROGRESSBAR_MINIMUM_UPDATE_INTERVAL', 'eggs') with caplog.at_level(logging.WARNING, logger='progressbar.bar'): for _ in range(3): progressbar.ProgressBar(fd=io.StringIO(), max_value=10) records = [r for r in caplog.records if 'eggs' in r.getMessage()] assert len(records) == 1 + + +def test_bad_interval_warning_is_once_per_value_beyond_sixteen_and_threadsafe( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + import logging + import threading + + from progressbar import bar as bar_module + + bar_module._reset_bad_interval_warnings() + with caplog.at_level(logging.WARNING, logger='progressbar.bar'): + for i in range(17): + bar_module._warn_bad_minimum_update_interval(f'bad-{i}') + bar_module._warn_bad_minimum_update_interval('bad-0') + barrier: threading.Barrier = threading.Barrier(4) + + def warn() -> None: + barrier.wait() + bar_module._warn_bad_minimum_update_interval('concurrent') + + threads: list[threading.Thread] = [ + threading.Thread(target=warn) for _ in range(4) + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + messages: list[str] = [r.getMessage() for r in caplog.records] + assert sum("'bad-0'" in m for m in messages) == 1 + assert sum("'concurrent'" in m for m in messages) == 1 + assert len(messages) == 18 + + +def test_desc_prefix_is_a_format_label_that_renders_literally() -> None: + fd: io.StringIO = io.StringIO() + bar: progressbar.ProgressBar = progressbar.ProgressBar( + fd=fd, + max_value=10, + desc='{files}', + widgets=[progressbar.SimpleProgress()], + ) + assert bar.prefix == '{files}: ' # read back unescaped, as in 4.6.0 + bar.start() + assert isinstance(bar.widgets[0], progressbar.FormatLabel) # as in 4.6.0 + bar.update(4, force=True) + assert '{files}: 4 of 10' in fd.getvalue() + + +def test_desc_with_braces_renders_literally_on_the_fast_path() -> None: + fd: io.StringIO = io.StringIO() + bar: progressbar.ProgressBar = progressbar.ProgressBar( + fd=fd, max_value=10, desc='{files}' + ) + bar.start() + bar.update(4, force=True) + assert '{files}: ' in fd.getvalue() + assert '{{files}}' not in fd.getvalue() + fast: progressbar.FastProgressBar = progressbar.FastProgressBar( + fd=io.StringIO(), max_value=10, desc='{files}' + ) + assert fast.prefix == '{files}: ' + from progressbar import fast as fast_module + + line: str = fast_module._pure_format_fast_line(fast) + assert line.startswith('{files}: ') + + +def test_frame_walks_are_refused_like_dunders() -> None: + import sys + import types + + bar, data = _label_data() + frame: types.FrameType = sys._getframe() + data['variables'] = progressbar.utils.AttributeDict(frame=frame) + for template in ( + '{variables.frame.f_globals}', + '{variables.frame.f_locals}', + '{variables.frame.f_back}', + ): + label = progressbar.FormatLabel(template, new_style=True) + with pytest.raises(ValueError, match='not allowed'): + label(bar, data) From cd11b16005b27bec3a02ea8f31d08bad4739d1a1 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Sun, 25 Oct 2026 14:02:38 +0100 Subject: [PATCH 10/24] Close the remaining ways the CLI could truncate or exit non-zero Refuse an -o path that is the file stdin is open on, an unreadable input and a closed stdin before the output is opened, so the output is never truncated for an invocation that then fails. Reject a size option above sys.maxsize instead of raising OverflowError from read(). Flush the output inside the guarded block, so a broken pipe that is only noticed when a small copy's buffer reaches the pipe exits 0 like a large one. --- progressbar/__main__.py | 36 ++++++++++++- tests/test_progressbar_command.py | 86 ++++++++++++++++++++++++++++++- 2 files changed, 119 insertions(+), 3 deletions(-) diff --git a/progressbar/__main__.py b/progressbar/__main__.py index d64d4c25..c04b09c1 100644 --- a/progressbar/__main__.py +++ b/progressbar/__main__.py @@ -414,6 +414,8 @@ def _parse_size_option( parser.error(f'invalid {option} value {value!r}: {exception}') if size < minimum: parser.error(f'{option} must be at least {minimum}, got {value!r}') + if size > sys.maxsize: + parser.error(f'{option} is too large: {value!r}') return size @@ -432,6 +434,8 @@ def _collect_inputs( filesize_available: bool = True for filename in args.input: if filename == '-': + if sys.stdin is None: # started with stdin closed + parser.error('stdin is closed, cannot read from -') inputs.append(sys.stdin if args.line_mode else sys.stdin.buffer) filesize_available = False continue @@ -441,6 +445,8 @@ def _collect_inputs( parser.error(f'File not found: {filename}') if input_path.is_dir(): parser.error(f'Is a directory: {filename}') + if not os.access(input_path, os.R_OK): + parser.error(f'Permission denied: {filename}') total_size += input_path.stat().st_size inputs.append(input_path) return inputs, total_size, filesize_available @@ -466,11 +472,33 @@ def _check_output_path( # Devices (two names of one tty, /dev/null) share an inode without # sharing data, so only a regular file can be truncated away return - for input_path in inputs: - if isinstance(input_path, Path) and output_path.samefile(input_path): + output_stat: os.stat_result = output_path.stat() + for input_item in inputs: + same: bool = ( + output_path.samefile(input_item) + if isinstance(input_item, Path) + else _stream_is_file(input_item, output_stat) + ) + if same: parser.error(f'Output file is also an input: {output}') +def _stream_is_file(stream: IO[typing.Any], target: os.stat_result) -> bool: + """Return whether `stream` (stdin, say) is open on the file `target`. + + A stream without a descriptor (`io.BytesIO`, a test double) is not + an alias of anything. + """ + try: + stream_stat: os.stat_result = os.fstat(stream.fileno()) + except (AttributeError, OSError, ValueError): + return False + return (stream_stat.st_dev, stream_stat.st_ino) == ( + target.st_dev, + target.st_ino, + ) + + def _open_input( input_path: IO[typing.Any] | Path, line_mode: bool, @@ -613,6 +641,10 @@ def main(argv: list[str] | None = None) -> None: started_at, transferred, ) + # A reader that went away is only noticed when the buffer + # reaches the pipe, which for a small copy is this flush, so + # it must happen inside the guarded block + output_stream.flush() except KeyboardInterrupt: exit_status = EXIT_INTERRUPTED except BrokenPipeError: diff --git a/tests/test_progressbar_command.py b/tests/test_progressbar_command.py index b991bd4a..6292f6d2 100644 --- a/tests/test_progressbar_command.py +++ b/tests/test_progressbar_command.py @@ -1,3 +1,4 @@ +import argparse import io import pathlib @@ -331,7 +332,14 @@ def test_output_same_as_input_is_rejected(tmp_path: pathlib.Path) -> None: @pytest.mark.parametrize( - 'option', [['-B', '0'], ['-B', 'abc'], ['-s', 'x'], ['-L', 'abc']] + 'option', + [ + ['-B', '0'], + ['-B', 'abc'], + ['-s', 'x'], + ['-L', 'abc'], + ['-B', '99999999999999999999999'], # OverflowError in read() before + ], ) def test_invalid_size_options_are_rejected( tmp_path: pathlib.Path, option: list[str] @@ -529,3 +537,79 @@ def fileno(self) -> int: monkeypatch.setattr(main.sys, 'stdout', io.StringIO()) main._discard_broken_stdout() # no fileno(): nothing to do, no raise + + +def test_output_that_is_also_stdin_is_rejected( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import io + + file: pathlib.Path = tmp_path / 'data.bin' + file.write_bytes(b'preserve this') + with file.open('rb') as stdin_file: + with pytest.raises(SystemExit) as excinfo: + main._check_output_path( + argparse.ArgumentParser(), str(file), [stdin_file] + ) + assert excinfo.value.code == 2 + assert file.read_bytes() == b'preserve this' + # a stream without a descriptor is not an alias of anything + main._check_output_path( + argparse.ArgumentParser(), str(file), [io.BytesIO()] + ) + + +def test_broken_pipe_found_at_flush_time_is_handled( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import io + + file: pathlib.Path = tmp_path / 'small.bin' + file.write_bytes(b'12345') + + class LateBrokenPipe(io.BytesIO): + def flush(self) -> None: + raise BrokenPipeError + + stream: LateBrokenPipe = LateBrokenPipe() + discarded: list[bool] = [] + monkeypatch.setattr(main, '_get_output_stream', lambda *args: stream) + monkeypatch.setattr(main.sys, 'stdout', stream) + monkeypatch.setattr( + main, '_discard_broken_stdout', lambda: discarded.append(True) + ) + main.main([str(file)]) # must not raise, and must not exit non-zero + assert discarded == [True] + + +def test_closed_stdin_is_a_usage_error( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + output: pathlib.Path = tmp_path / 'out.bin' + output.write_bytes(b'precious') + monkeypatch.setattr(main.sys, 'stdin', None) # `exec 0<&-` shape + with pytest.raises(SystemExit) as excinfo: + main.main(['-', '-o', str(output)]) + assert excinfo.value.code == 2 + assert output.read_bytes() == b'precious' + + +def test_unreadable_input_leaves_the_output_untouched( + tmp_path: pathlib.Path, +) -> None: + import os + + if os.name == 'nt' or os.geteuid() == 0: + pytest.skip('needs POSIX permissions and a non-root user') + secret: pathlib.Path = tmp_path / 'secret.txt' + secret.write_bytes(b'hidden') + secret.chmod(0) + output: pathlib.Path = tmp_path / 'out.txt' + output.write_bytes(b'precious') + try: + with pytest.raises(SystemExit) as excinfo: + main.main(['-q', str(secret), '-o', str(output)]) + finally: + secret.chmod(0o600) + assert excinfo.value.code == 2 + assert output.read_bytes() == b'precious' From b448a0165804e8a3d312ed0f74c04e017c20aa32 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Sun, 25 Oct 2026 14:02:39 +0100 Subject: [PATCH 11/24] Tolerate a MultiBar bar removed while its finished frame renders The render thread snapshots the bars and a worker may delete one (task_finished) before the finished-bar path runs. That path now returns early for a bar that is no longer registered, undoes its timestamp if the bar vanished while it was stamped, and ignores a concurrent removal when expiring it, instead of leaking the entry or raising KeyError from the render thread. --- progressbar/multi.py | 20 ++++++++++++++++---- tests/test_multibar.py | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/progressbar/multi.py b/progressbar/multi.py index d3a4b6af..d113500b 100644 --- a/progressbar/multi.py +++ b/progressbar/multi.py @@ -10,6 +10,7 @@ from __future__ import annotations import collections.abc +import contextlib import enum import importlib import io @@ -456,17 +457,28 @@ def _render_finished_bar( Returns: Zero lines (dropped or hidden) or one rendered line. """ - if bar_ not in self._finished_at: - self._finished_at[bar_] = now + if self.get(bar_.label) is not bar_: + # Removed between the render snapshot and now (a task + # finished on another thread): nothing to draw, and nothing + # to remember about it + return + finished_at: float | None = self._finished_at.get(bar_) + if finished_at is None: + finished_at = self._finished_at.setdefault(bar_, now) + if self.get(bar_.label) is not bar_: # pragma: no cover + # Removed while we stamped it: undo the stamp + self._finished_at.pop(bar_, None) + return # Force update to get the finished format update(write=False) if ( self.remove_finished and expired is not None - and expired >= self._finished_at[bar_] + and expired >= finished_at ): - del self[bar_.label] + with contextlib.suppress(KeyError): # removed concurrently + del self[bar_.label] return if not self.show_finished: diff --git a/tests/test_multibar.py b/tests/test_multibar.py index 9b6d66fd..bec2c712 100644 --- a/tests/test_multibar.py +++ b/tests/test_multibar.py @@ -577,3 +577,21 @@ def test_multibar_concurrent_mutation() -> None: assert not errors assert not multibar._thread or not multibar._thread.is_alive() + + +def test_render_of_a_bar_removed_mid_frame_does_not_leak() -> None: + # The render thread snapshots the bars, then a worker deletes one + # (task_finished); the finished-bar path must not stamp it back + # into _finished_at after __delitem__ popped it. + multibar = progressbar.MultiBar(fd=io.StringIO()) + bar_ = multibar['gone'] + bar_.start() + bar_.finish() + del multibar['gone'] + lines = list( + multibar._render_finished_bar( + bar_, time.monotonic(), None, lambda **kw: None + ) + ) + assert lines == [] + assert bar_ not in multibar._finished_at From 629f35d34f6d6d21390e380850f8ea0dad94ca82 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Sun, 25 Oct 2026 14:02:39 +0100 Subject: [PATCH 12/24] Read an annotated __version__ in the release guard and anchor stray names The version check accepted only `__version__ = '...'`; the annotated spelling `__version__: str = '...'` failed the guard although the package built. The stray-file pattern matched any path containing .orig, .rej or .bak; it now matches those as file extensions only. --- .github/workflows/release.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 32732927..83b75401 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -51,7 +51,7 @@ jobs: echo "::error::tag '$tag' does not look like vX.Y.Z" exit 1 fi - version="$(sed -n "s/^__version__ = ['\"]\([^'\"]*\)['\"].*/\1/p" progressbar/__about__.py)" + version="$(sed -n -E "s/^__version__(: *str)? = ['\"]([^'\"]*)['\"].*/\2/p" progressbar/__about__.py)" if [ -z "$version" ]; then echo "::error::could not read __version__ from progressbar/__about__.py" exit 1 @@ -81,7 +81,7 @@ jobs: test "$(echo "$unpacked" | grep -cvE '^progressbar$|^progressbar2-.*\.dist-info$')" -eq 0 sdist="$(tar tzf dist/*.tar.gz)" echo "$sdist" - stray_pattern='\.sync-conflict-|\.orig|\.rej|\.bak' + stray_pattern='\.sync-conflict-|\.(orig|rej|bak)$' if { unzip -Z1 dist/*.whl; echo "$sdist"; } | grep -E "$stray_pattern"; then echo "::error::distribution contains stray conflict or backup files" exit 1 From 7f11953c270da7ed6c24cddd63a738778b6b1285 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Sun, 25 Oct 2026 14:02:39 +0100 Subject: [PATCH 13/24] Make the imap_unordered ordering test independent of timing The test relied on 30 ms sleep differences between five workers and failed twice on a loaded machine. Only one item can now complete until a gate opens, so the first yielded pair is deterministic. --- tests/test_parallel_imap.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tests/test_parallel_imap.py b/tests/test_parallel_imap.py index c020e372..5aff254c 100644 --- a/tests/test_parallel_imap.py +++ b/tests/test_parallel_imap.py @@ -74,14 +74,22 @@ def test_on_error_raise_raises_at_iteration(self) -> None: class TestImapUnordered: @pytest.mark.no_freezegun def test_yields_pairs_in_completion_order(self) -> None: - pairs: list[tuple[int, int]] = list( - _sync.imap_unordered( - _sleep_inverse, range(5), workers=5, bar=False - ) - ) + # Deterministic ordering: only item 4 can finish until the gate + # opens, so timing on a loaded machine cannot reorder the result + gate: threading.Event = threading.Event() + + def gated(value: int) -> int: + if value != 4: + assert gate.wait(10) + return value + + iterator = _sync.imap_unordered(gated, range(5), workers=5, bar=False) + first: tuple[int, int] = next(iterator) + gate.set() + pairs: list[tuple[int, int]] = [first, *iterator] assert sorted(pairs) == [(value, value) for value in range(5)] - # The fastest item (highest input) completes and is seen first. - assert pairs[0] == (4, 4) + # The only item that could complete is seen first. + assert first == (4, 4) def test_multi_iterable_pairs_use_args_tuple(self) -> None: pairs = list( From 517d16de6628dbb5340d9669b1e78c66c4defff0 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Sun, 25 Oct 2026 15:09:50 +0100 Subject: [PATCH 14/24] Finish the deep-copied bars in the stream pickle test test_wrapping_io_and_a_redirecting_bar_deepcopy_and_pickle deep-copied a started, redirecting bar and a StreamWrapper holding one, and dropped the copies. A copy of a started bar is itself a started bar, so its __del__ ran finish() whenever the gc got to it, usually inside a later test, where it dropped the singleton's capture count and restored sys.stdout under that test's feet. That is what made test_redirected_stderr_lines_are_flushed_above_bar fail now and then under coverage. The copies are marked finished before they go. --- tests/test_stream_security.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py index de0f8c9f..e22804fa 100644 --- a/tests/test_stream_security.py +++ b/tests/test_stream_security.py @@ -224,10 +224,14 @@ def test_wrapping_io_and_a_redirecting_bar_deepcopy_and_pickle( ) bar.start() try: - copy.deepcopy(bar) - copy.deepcopy(streams) # while wrapping + clones: list[object] = [copy.deepcopy(bar), copy.deepcopy(streams)] finally: bar.finish() + # A copy of a started bar is a started bar: its __del__ would finish() + # it later, at gc time, and unwrap the singleton under some other test + for clone in clones: + for listener in getattr(clone, 'listeners', [clone]): + listener._finished = True def test_captured_write_never_waits_for_the_streamwrapper_lock() -> None: From 852967934dbc402d4fa9e9635c4116bed522b72d Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Sun, 25 Oct 2026 15:25:23 +0100 Subject: [PATCH 15/24] Deliver the other stream's text when one original stream fails A flush takes the buffered text of both wrapped streams and then writes it to the two original streams. When the stdout write raised, the exception left the loop before the stderr text was written, and that text was already gone from the buffer, so a broken stdout cost the stderr lines of the same round. Every taken text is now written before the first exception is raised. Delivery stays as 4.6.0 did it: each caller writes what it took, at once, with no lock held during the writes. A single ordered delivery queue was tried first and taken back again, in both a non-blocking shape (a caller returned with its own text still queued behind another thread, so text written right after a stop landed above it, and a flush during a render thread's delivery wrote nothing) and a blocking shape (a lock-order cycle with a logging handler whose stream is the wrapper, when the original stream itself logs). Working code notices: an exception from the stdout target no longer drops the stderr text flushed in the same call. --- progressbar/utils.py | 22 +++++++++ tests/test_stream_security.py | 93 ++++++++++++++++++++++++++++++++++- 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/progressbar/utils.py b/progressbar/utils.py index 42ce09c3..b7c6c541 100644 --- a/progressbar/utils.py +++ b/progressbar/utils.py @@ -628,9 +628,31 @@ def _deliver(self, pending: list[_Pending]) -> None: warning is logged. `sys.stdout`/`sys.stderr` are left installed as-is: only further flush attempts are skipped, not the wrapping itself. + + Any other exception from a stream propagates to the caller, but + only after the other stream's text has been delivered too: the + text was already taken from the buffers, so a stdout that fails + must not cost the stderr lines of the same round. """ + failure: BaseException | None = None for name, wrapper, text in pending: + exception: BaseException | None = self._deliver_guarded( + name, wrapper, text + ) + if failure is None: + failure = exception + if failure is not None: + raise failure + + def _deliver_guarded( + self, name: str, wrapper: WrappingIO, text: str + ) -> BaseException | None: + """Deliver one wrapper's text, returning what it raised.""" + try: self._deliver_one(name, wrapper, text) + except BaseException as exception: # noqa: BLE001 + return exception + return None def _deliver_one(self, name: str, wrapper: WrappingIO, text: str) -> None: """Deliver one wrapper's text, disabling its redirection on failure.""" diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py index e22804fa..769f2e58 100644 --- a/tests/test_stream_security.py +++ b/tests/test_stream_security.py @@ -529,7 +529,7 @@ def getvalue(self) -> str: value: str = super().getvalue() if threading.current_thread().name == 'first-stop': paused.set() - assert resume.wait(3) + assert resume.wait(10) return value streams: utils.StreamWrapper = utils.StreamWrapper() @@ -544,7 +544,7 @@ def getvalue(self) -> str: target=streams.stop_capturing, name='first-stop' ) first.start() - assert paused.wait(2) + assert paused.wait(10) second_done: threading.Event = threading.Event() def another_lifecycle() -> None: @@ -670,3 +670,92 @@ def test_shared_lock_box_reset_and_wrapper_lock_setter() -> None: private: threading.Lock = threading.Lock() wrapper.lock = private # the 4.6.0-era way of handing in a lock assert wrapper.lock is private + + +def test_stop_capturing_returns_with_its_own_text_delivered( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # Delivery is synchronous, as in 4.6.0: text written right after a + # stop lands below the text that stop flushed, even while another + # thread is still inside a delivery of its own + paused: threading.Event = threading.Event() + resume: threading.Event = threading.Event() + + class PausingTarget(io.StringIO): + def write(self, text: str) -> int: + written: int = super().write(text) + if threading.current_thread().name == 'first-stop': + paused.set() + assert resume.wait(10) + return written + + streams: utils.StreamWrapper = utils.StreamWrapper() + target: PausingTarget = PausingTarget() + streams.stdout = streams.original_stdout = target + monkeypatch.setattr(sys, 'stdout', target) + wrapper: utils.WrappingIO = streams.wrap_stdout() + streams.start_capturing() + wrapper.write('spam|') + first: threading.Thread = threading.Thread( + target=streams.stop_capturing, name='first-stop' + ) + first.start() + assert paused.wait(10) + streams.start_capturing() + wrapper.write('eggs|') + streams.stop_capturing() # delivers 'eggs|' itself, without waiting + wrapper.write('after-stop|') # passes straight through, capturing is off + resume.set() + first.join() + streams.unwrap_stdout() + assert target.getvalue() == 'spam|eggs|after-stop|' + + +def test_target_that_flushes_the_streams_while_written_does_not_deadlock( + monkeypatch: pytest.MonkeyPatch, +) -> None: + streams: utils.StreamWrapper = utils.StreamWrapper() + + class FlushingTarget(io.StringIO): + # A target that calls back into the streams on the same thread, + # as a bar-aware widget or a logging handler might + def write(self, text: str) -> int: + written: int = super().write(text) + streams.flush() + return written + + target: FlushingTarget = FlushingTarget() + streams.stdout = streams.original_stdout = target + monkeypatch.setattr(sys, 'stdout', target) + streams.wrap_stdout() + streams.start_capturing() + print('spam', end='') + streams.stop_capturing() + streams.unwrap_stdout() + assert target.getvalue() == 'spam' + + +def test_failed_stdout_delivery_still_delivers_the_stderr_text( + monkeypatch: pytest.MonkeyPatch, +) -> None: + class Broken(io.StringIO): + def write(self, text: str) -> int: + raise OSError('gone') + + streams: utils.StreamWrapper = utils.StreamWrapper() + out: Broken = Broken() + err: io.StringIO = io.StringIO() + streams.stdout = streams.original_stdout = out + streams.stderr = streams.original_stderr = err + monkeypatch.setattr(sys, 'stdout', out) + monkeypatch.setattr(sys, 'stderr', err) + streams.wrap_stdout() + streams.wrap_stderr() + streams.start_capturing() + print('spam', end='') + print('eggs', end='', file=sys.stderr) + with pytest.raises(OSError, match='gone'): + streams.stop_capturing() + assert err.getvalue() == 'eggs' # not lost to the stdout failure + streams.unwrap_stdout() + streams.unwrap_stderr() From 224d51c2602efa9ee7e7e9a24444fba78bffcc61 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Wed, 28 Oct 2026 01:41:12 +0100 Subject: [PATCH 16/24] Restore the SIGWINCH handler however the last bar goes away The shared resize registry saved the previous handler when its set of bars was empty and restored it from the last uninstall(). Three ways out of a bar skipped that restore: a bar collected without finish() left the registry's handler installed for the rest of the process, a finish() off the main thread hit the ValueError from signal.signal() with the bar already discarded, after which the next bar saved the registry's own handler as the "previous" one and the real one was lost for good, and a handler installed from C (getsignal() returns None) made the restore a TypeError. One bar whose _handle_resize() raised also skipped the other bars and propagated out of the signal handler into unrelated code. Whether the shared handler is installed is now read from getsignal() instead of inferred from the set, so a handler that other code set over ours is never clobbered. The saved handler is cleared only after a restore succeeds, a restore that fails off the main thread is retried at the next registration change or the next resize signal, None restores SIG_DFL (SIGWINCH is ignored by default), every registered bar gets a weakref finalizer that restores once no live bar is left, and a failing bar is logged at WARNING on the progressbar.bar logger and skipped. What working code can notice: a resize handler that raised now logs instead of raising, and the previous handler comes back after bars that were dropped without finish() or finished from a worker thread. --- progressbar/bar.py | 77 ++++++++++++++++-- tests/test_progressbar.py | 167 ++++++++++++++++++++++++++++++-------- 2 files changed, 200 insertions(+), 44 deletions(-) diff --git a/progressbar/bar.py b/progressbar/bar.py index e92e00c4..5e70a7a5 100644 --- a/progressbar/bar.py +++ b/progressbar/bar.py @@ -590,18 +590,38 @@ class _ResizeRegistry: A single signal handler dispatches to every live bar. The original handler is saved when the first bar registers and restored when the last one unregisters, so overlapping bars can finish in any order - without leaving a dangling handler installed. + without leaving a dangling handler installed. A bar that is garbage + collected without `finish()` counts as unregistered too. + + `signal.signal` only works on the main thread, so a last bar + finished elsewhere leaves the shared handler in place with nobody + to dispatch to. The saved handler is kept for that case, and the + restore is retried at the next registration change or the next + resize signal (which does run on the main thread). Whether the + shared handler is installed is read from `signal.getsignal` rather + than remembered, so a handler that other code set over it is never + clobbered by a restore. """ bars: typing.ClassVar[weakref.WeakSet[ResizableMixin]] = weakref.WeakSet() previous_handler: typing.ClassVar[typing.Any] = None + @classmethod + def installed(cls) -> bool: + """Return whether `handle_resize` is the handler set on SIGWINCH.""" + import signal + + return ( + signal.getsignal(signal.SIGWINCH) # type: ignore[attr-defined] + == cls.handle_resize + ) + @classmethod def install(cls, bar: ResizableMixin) -> None: """Register `bar` for resize dispatch, installing the handler once. The process-global `SIGWINCH` handler is only installed (saving - whatever handler was previously set) on the first registration. A + whatever handler was previously set) when it is not ours yet. A second, third, etc. overlapping bar just joins the `WeakSet`. """ import signal @@ -610,7 +630,7 @@ def install(cls, bar: ResizableMixin) -> None: # Not available on Windows return - if not cls.bars: + if not cls.installed(): cls.previous_handler = signal.getsignal( signal.SIGWINCH # type: ignore[attr-defined] ) @@ -620,6 +640,7 @@ def install(cls, bar: ResizableMixin) -> None: ) cls.bars.add(bar) + weakref.finalize(bar, cls._restore_if_idle) @classmethod def uninstall(cls, bar: ResizableMixin) -> None: @@ -630,27 +651,65 @@ def uninstall(cls, bar: ResizableMixin) -> None: keep the shared handler installed regardless of registration order. """ + cls.bars.discard(bar) + cls._restore_if_idle() + + @classmethod + def _restore_if_idle(cls) -> None: + """Put the saved handler back when no live bar is registered. + + Iterating the `WeakSet` skips entries whose bar is already dead + but not yet removed, which is the state inside a finalizer. + """ import signal if not hasattr(signal, 'SIGWINCH'): # pragma: no cover # Not available on Windows return - - cls.bars.discard(bar) - if not cls.bars: + if not cls.installed() or any(True for _ in cls.bars): + return + handler = cls.previous_handler + if handler is None: + # `getsignal` returns None for a handler installed from C, + # which cannot be reinstalled from Python. SIGWINCH is + # ignored by default, so the default is the closest fit. + handler = signal.SIG_DFL + try: signal.signal( signal.SIGWINCH, # type: ignore[attr-defined] - cls.previous_handler, + handler, ) - cls.previous_handler = None + except ValueError: + # Not the main thread: keep `previous_handler` and retry at + # the next registration change or resize signal + return + cls.previous_handler = None @classmethod def handle_resize( cls, signum: int | None = None, frame: FrameType | None = None ) -> None: - """Dispatch a `SIGWINCH` to every currently-registered bar.""" + """Dispatch a `SIGWINCH` to every currently-registered bar. + + A bar whose `_handle_resize` raises is logged and skipped, so + the other bars still get the resize and nothing propagates out + of the signal handler into unrelated main-thread code. + """ for bar in list(cls.bars): + cls._dispatch(bar, signum, frame) + cls._restore_if_idle() + + @staticmethod + def _dispatch( + bar: ResizableMixin, signum: int | None, frame: FrameType | None + ) -> None: + """Resize one bar, logging instead of raising when it fails.""" + try: bar._handle_resize(signum, frame) + except Exception: + logger.warning( + 'Ignoring a failed resize of %r', bar, exc_info=True + ) class ResizableMixin(ProgressBarMixinBase): diff --git a/tests/test_progressbar.py b/tests/test_progressbar.py index 04fe678b..c46a451e 100644 --- a/tests/test_progressbar.py +++ b/tests/test_progressbar.py @@ -1,6 +1,7 @@ import contextlib import gc import io +import logging import os import signal import sys @@ -344,47 +345,143 @@ def fail_finish(self): @pytest.mark.skipif(os.name == 'nt', reason='SIGWINCH is POSIX-only') -def test_sigwinch_restored_with_overlapping_bars() -> None: +def test_sigwinch_restored_with_overlapping_bars( + isolated_resize_registry, +) -> None: # Regression: A5 - with two live bars, finishing them in creation - # order left a dangling handler installed. + # order left a dangling handler installed. The fixture starts from + # SIG_IGN, so "still installed" and "restored" can be told apart. + bar1 = progressbar.ProgressBar(max_value=5, fd=io.StringIO()) + bar1.start() + bar2 = progressbar.ProgressBar(max_value=5, fd=io.StringIO()) + bar2.start() + + # The first bar installs the shared handler + assert signal.getsignal(signal.SIGWINCH) is not signal.SIG_IGN + + # A resize signal is dispatched to all live bars + signal.raise_signal(signal.SIGWINCH) + assert isinstance(bar1.term_width, int) + assert isinstance(bar2.term_width, int) + + bar1.update(5) + bar1.finish() + # The handler must stay installed while bar2 is still live + assert signal.getsignal(signal.SIGWINCH) is not signal.SIG_IGN + + bar2.update(5) + bar2.finish() + # The last bar to finish restores the previous handler + assert signal.getsignal(signal.SIGWINCH) is signal.SIG_IGN + + +@pytest.fixture +def isolated_resize_registry(): + """Give a test an empty `_ResizeRegistry` and the handler it found. + + The registry is process-global, so bars left registered by other + tests (and a handler they left installed) would otherwise leak into + the assertions. Everything is put back afterwards. + """ import progressbar.bar as bar_module + registry = bar_module._ResizeRegistry saved_handler = signal.getsignal(signal.SIGWINCH) - # Isolate the global registry so the assertions don't depend on bars - # left registered (and a handler left installed) by other tests - saved_bars = list(bar_module._ResizeRegistry.bars) - saved_prev = bar_module._ResizeRegistry.previous_handler - bar_module._ResizeRegistry.bars.clear() - bar_module._ResizeRegistry.previous_handler = None - - # Start from a known sentinel handler so we can tell apart "still - # installed" from "restored" without depending on global state + saved_bars = list(registry.bars) + saved_prev = registry.previous_handler + registry.bars.clear() + registry.previous_handler = None signal.signal(signal.SIGWINCH, signal.SIG_IGN) try: - bar1 = progressbar.ProgressBar(max_value=5, fd=io.StringIO()) - bar1.start() - bar2 = progressbar.ProgressBar(max_value=5, fd=io.StringIO()) - bar2.start() - - # The first bar installs the shared handler - assert signal.getsignal(signal.SIGWINCH) is not signal.SIG_IGN - - # A resize signal is dispatched to all live bars - signal.raise_signal(signal.SIGWINCH) - assert isinstance(bar1.term_width, int) - assert isinstance(bar2.term_width, int) - - bar1.update(5) - bar1.finish() - # The handler must stay installed while bar2 is still live - assert signal.getsignal(signal.SIGWINCH) is not signal.SIG_IGN - - bar2.update(5) - bar2.finish() - # The last bar to finish restores the previous handler - assert signal.getsignal(signal.SIGWINCH) is signal.SIG_IGN + yield registry finally: + registry.bars.clear() for restored_bar in saved_bars: - bar_module._ResizeRegistry.bars.add(restored_bar) - bar_module._ResizeRegistry.previous_handler = saved_prev + registry.bars.add(restored_bar) + registry.previous_handler = saved_prev signal.signal(signal.SIGWINCH, saved_handler) + + +@pytest.mark.skipif(os.name == 'nt', reason='SIGWINCH is POSIX-only') +def test_sigwinch_dispatch_survives_a_raising_bar( + isolated_resize_registry, caplog +) -> None: + # A35: one bar's failing `_handle_resize` skipped every later bar + # and raised out of the signal handler into whatever the main + # thread was doing. + class Broken(progressbar.ProgressBar): + def _handle_resize(self, signum=None, frame=None) -> None: + if signum is not None: + raise RuntimeError('spam') + super()._handle_resize(signum, frame) + + broken = Broken(max_value=5, fd=io.StringIO()) + broken.start() + other = progressbar.ProgressBar(max_value=5, fd=io.StringIO()) + other.start() + other.term_width = 1 + try: + with caplog.at_level(logging.WARNING, logger='progressbar.bar'): + signal.raise_signal(signal.SIGWINCH) + assert other.term_width != 1 + assert any( + record.levelno == logging.WARNING and record.exc_info + for record in caplog.records + ) + finally: + broken.finish() + other.finish() + + +@pytest.mark.skipif(os.name == 'nt', reason='SIGWINCH is POSIX-only') +def test_sigwinch_restored_after_a_leaked_bar( + isolated_resize_registry, +) -> None: + # A35: a bar that was never finished dropped out of the WeakSet, + # and nobody restored the handler it had installed. Registration + # happens at construction, so a bar that is built and dropped + # without start() is the shape that gets collected (a started bar + # is kept alive by `streams.listeners` until it finishes). + bar = progressbar.ProgressBar(max_value=5, fd=io.StringIO()) + assert signal.getsignal(signal.SIGWINCH) is not signal.SIG_IGN + bar._finished = True # keep __del__ from finishing it for us + del bar + gc.collect() + assert signal.getsignal(signal.SIGWINCH) is signal.SIG_IGN + + +@pytest.mark.skipif(os.name == 'nt', reason='SIGWINCH is POSIX-only') +def test_sigwinch_previous_handler_survives_an_off_thread_finish( + isolated_resize_registry, +) -> None: + # A35: `signal.signal` raises ValueError off the main thread, the + # bar was already unregistered, and the next bar then saved the + # registry's own handler as the "previous" one, losing SIG_IGN. + import threading + + first = progressbar.ProgressBar(max_value=5, fd=io.StringIO()) + first.start() + thread = threading.Thread(target=first.finish) + thread.start() + thread.join(10) + assert not thread.is_alive() + + second = progressbar.ProgressBar(max_value=5, fd=io.StringIO()) + second.start() + second.finish() + assert signal.getsignal(signal.SIGWINCH) is signal.SIG_IGN + + +@pytest.mark.skipif(os.name == 'nt', reason='SIGWINCH is POSIX-only') +def test_sigwinch_restores_the_default_when_no_python_handler_was_set( + isolated_resize_registry, +) -> None: + # A35: `signal.getsignal` returns None for a handler installed from + # C, and `signal.signal(SIGWINCH, None)` is a TypeError, so the + # registry's handler stayed installed for good. + registry = isolated_resize_registry + bar = progressbar.ProgressBar(max_value=5, fd=io.StringIO()) + bar.start() + registry.previous_handler = None + bar.finish() + assert signal.getsignal(signal.SIGWINCH) is signal.SIG_DFL From fe620af78f5b90b54aba3d077a6b23ecbc7cfffd Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Wed, 28 Oct 2026 01:41:12 +0100 Subject: [PATCH 17/24] Keep a captured write from being lost while the buffer drains WrappingIO.write() appended to the StringIO buffer without a lock while _take() read it, then truncated it. A write landing between the read and the truncate was gone: deterministic with a buffer that pauses in truncate(), and a handful of tokens per run in a six-thread stress on the free-threaded builds. 4.6.0 has the same race. Each WrappingIO now holds a reentrant lock in a shared-lock box that write() and _take() take around the StringIO write, getvalue, seek and truncate calls only. It is never held while a listener, the target stream or the buffer's own flush() is called (a review found that a replacement buffer whose flush() logs through a handler on the wrapper deadlocked when the flush ran inside the lock), so it cannot take part in a lock-order cycle with a logging handler. The box survives pickle, deepcopy and fork like the listeners lock does, and the class-level default keeps a subclass that skips super().__init__() working. --- progressbar/utils.py | 28 ++++++--- tests/test_stream_security.py | 103 ++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 7 deletions(-) diff --git a/progressbar/utils.py b/progressbar/utils.py index b7c6c541..5b07fc35 100644 --- a/progressbar/utils.py +++ b/progressbar/utils.py @@ -251,6 +251,12 @@ class WrappingIO: #: default keeps a subclass that skips `super().__init__()` working, #: as for `StreamWrapper`. _lock_box: _SharedLock = _SharedLock() + #: Holds the lock that keeps `write()` and `_take()` from + #: interleaving on `buffer`: a write between the read and the + #: truncate of a drain was lost. Held around the `io.StringIO` + #: write, getvalue, seek and truncate calls only, never while + #: calling a listener, `target` or the buffer's `flush()`. + _buffer_box: _SharedLock = _SharedLock(threading.RLock) needs_clear: bool = False @property @@ -297,6 +303,7 @@ def __init__( self._lock_box = lock else: self._lock_box = _SharedLock(threading.Lock, lock) + self._buffer_box = _SharedLock(threading.RLock) self.needs_clear = False def write(self, value: str) -> int: @@ -317,7 +324,8 @@ def write(self, value: str) -> int: """ ret = 0 if self.capturing: - ret += self.buffer.write(value) + with self._buffer_box.lock: + ret += self.buffer.write(value) if '\n' in value: # pragma: no branch self.needs_clear = True # Iterate a snapshot taken under the lock: a listener's @@ -362,14 +370,20 @@ def _take(self) -> str: overlapping flushes cannot both pick up the same text, and delivers the result after releasing it. """ - value: str = self.buffer.getvalue() + with self._buffer_box.lock: + value: str = self.buffer.getvalue() + if value: + # Clear the buffer before writing so a failed write + # cannot cause the same data to be written again by the + # next flush + self.buffer.seek(0) + self.buffer.truncate(0) + self.needs_clear = False if value: + # Outside the lock: a replacement buffer's flush() may call + # out (log, say), and must not do so while holding a lock + # that write() needs self.flush() - # Clear the buffer before writing so a failed write cannot - # cause the same data to be written again by the next flush - self.buffer.seek(0) - self.buffer.truncate(0) - self.needs_clear = False return value def _deliver(self, value: str) -> None: diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py index 769f2e58..968666d2 100644 --- a/tests/test_stream_security.py +++ b/tests/test_stream_security.py @@ -759,3 +759,106 @@ def write(self, text: str) -> int: assert err.getvalue() == 'eggs' # not lost to the stdout failure streams.unwrap_stdout() streams.unwrap_stderr() + + +class _PausingStringIO(io.StringIO): + """A buffer whose first `truncate()` pauses until told to go on.""" + + def __init__(self) -> None: + super().__init__() + self.paused: threading.Event = threading.Event() + self.resume: threading.Event = threading.Event() + self.calls: int = 0 + + def truncate(self, size: int | None = None) -> int: + self.calls += 1 + if self.calls == 1: + self.paused.set() + assert self.resume.wait(10) + return super().truncate(size) + + +def test_write_during_a_drain_is_not_lost() -> None: + # Codex-4b finding 7: `_take()` read the buffer, then truncated it, + # and a `write()` that landed between the two was gone. + wrapper = utils.WrappingIO(io.StringIO(), capturing=True) + buffer = _PausingStringIO() + wrapper.buffer = buffer + wrapper.write('A|') + taken: list[str] = [] + + def take() -> None: + taken.append(wrapper._take()) + + taker = threading.Thread(target=take) + taker.start() + assert buffer.paused.wait(10) + writer = threading.Thread(target=wrapper.write, args=('B|',)) + writer.start() + writer.join(0.5) # blocked behind the drain, or done without a lock + buffer.resume.set() + taker.join(10) + writer.join(10) + assert not taker.is_alive() and not writer.is_alive() + + taken.append(wrapper._take()) + assert ''.join(taken) == 'A|B|' + + +def test_buffer_flush_that_logs_through_the_wrapper_does_not_deadlock() -> ( + None +): + # Review 5: `_take()` called `buffer.flush()` while holding the buffer + # lock. A replacement buffer whose flush() logs through a handler + # that writes to the wrapper then waited for the handler lock, held + # by a thread waiting to write into the buffer: a lock-order cycle + # 4.6.0 could not have. + entered: threading.Event = threading.Event() + handler_held: threading.Event = threading.Event() + logger: logging.Logger = logging.getLogger('test-buffer-flush-logs') + logger.propagate = False + logger.setLevel(logging.INFO) + + class LoggingBuffer(io.StringIO): + fired: bool = False + + def flush(self) -> None: + if not self.fired: + self.fired = True + entered.set() + logger.info('flush-log') + super().flush() + + wrapper = utils.WrappingIO(io.StringIO(), capturing=True) + wrapper.buffer = LoggingBuffer() + handler: logging.StreamHandler[typing.Any] = logging.StreamHandler(wrapper) + logger.addHandler(handler) + try: + wrapper.write('A|') + + def writer() -> None: + handler.acquire() + try: + handler_held.set() + assert entered.wait(10) + wrapper.write('B|') + finally: + handler.release() + + def drainer() -> None: + assert handler_held.wait(10) + wrapper._flush() + + threads = [ + threading.Thread(target=writer, daemon=True), + threading.Thread(target=drainer, daemon=True), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join(10) + assert not any(thread.is_alive() for thread in threads) + wrapper._flush() + assert wrapper.target.getvalue() == 'A|B|flush-log\n' + finally: + logger.removeHandler(handler) From d99a25463ce0d73881528d02b280104474f443cb Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Wed, 28 Oct 2026 01:41:12 +0100 Subject: [PATCH 18/24] Treat an -o path that aliases stdout like stdout on a broken pipe `progressbar FILE -o /dev/stdout | head -c1` exited 1. The file opened for -o is not sys.stdout, so the quiet exit for a reader that went away did not apply, and the interpreter reported the failing flush at exit. _output_is_stdout() now also accepts a file whose fstat() reports the same device and inode as sys.stdout, and _discard_broken_stdout() points both descriptor 1 and the file's own descriptor at os.devnull so neither the exit flush nor the exit stack's close raises again. Nothing changes unless descriptor 1 is involved: a replacement stdout on another descriptor, a regular output file and a FIFO given by path are left alone. --- progressbar/__main__.py | 61 +++++++++++++++++++++------- tests/test_progressbar_command.py | 67 ++++++++++++++++++++++++++++--- 2 files changed, 109 insertions(+), 19 deletions(-) diff --git a/progressbar/__main__.py b/progressbar/__main__.py index c04b09c1..20ed539e 100644 --- a/progressbar/__main__.py +++ b/progressbar/__main__.py @@ -551,26 +551,61 @@ def _copy_stream( _sleep_for_rate_limit(rate_limit, transferred, started_at) -def _discard_broken_stdout() -> None: - """Point fd 1 at `os.devnull` after a broken pipe on stdout. +def _output_is_stdout(output_stream: typing.IO[typing.Any]) -> bool: + """Return whether `output_stream` writes to the same place as stdout. + + `sys.stdout` and its binary buffer count, and so does a file opened + by `-o` on a path that aliases it (`/dev/stdout`, `/dev/fd/1`): its + descriptor differs, but `fstat` reports the same device and inode. + """ + if output_stream is sys.stdout or output_stream is getattr( + sys.stdout, 'buffer', None + ): + return True + try: + stdout_stat: os.stat_result = os.fstat(sys.stdout.fileno()) + except (AttributeError, OSError, ValueError): + return False + return _stream_is_file(output_stream, stdout_stat) + + +def _fileno(stream: typing.IO[typing.Any]) -> int | None: + """Return the descriptor behind `stream`, or None when it has none. + + An `io.StringIO` under a test runner has no descriptor, and a closed + file raises `ValueError` for one. + """ + try: + return stream.fileno() + except (OSError, ValueError, AttributeError): + return None + + +def _discard_broken_stdout(output_stream: typing.IO[typing.Any]) -> None: + """Point fd 1 (and an `-o` alias of it) at `os.devnull` after EPIPE. The copy loop has already stopped, but the interpreter flushes `sys.stdout` once more at exit, and that flush would raise the same `BrokenPipeError` again (status 120, "Exception ignored on flushing sys.stdout"). Redirecting the descriptor makes that last flush succeed, which is what the Python docs recommend for a pipeline - whose reader went away. + whose reader went away. A file opened on `/dev/stdout` is closed by + the exit stack with the same failing flush, so its descriptor is + redirected as well. """ - try: - fd: int = sys.stdout.fileno() - except (OSError, ValueError, AttributeError): - return # not descriptor-backed (io.StringIO under a test runner) - if fd != 1: - return # a replacement stdout owns that descriptor, leave it alone + descriptors: set[int] = { + fd + for fd in (_fileno(sys.stdout), _fileno(output_stream)) + if fd is not None + } + # A replacement stdout that owns another descriptor is left alone + if 1 not in descriptors: + return try: devnull: int = os.open(os.devnull, os.O_WRONLY) try: - os.dup2(devnull, fd) + for fd in descriptors: + os.dup2(devnull, fd) finally: os.close(devnull) except OSError: # pragma: no cover @@ -651,10 +686,8 @@ def main(argv: list[str] | None = None) -> None: # The reader went away (`... | head`): finish quietly with # status 0, as 4.6.0 did and as a pipeline under pipefail # expects - if output_stream is sys.stdout or output_stream is getattr( - sys.stdout, 'buffer', None - ): - _discard_broken_stdout() + if _output_is_stdout(output_stream): + _discard_broken_stdout(output_stream) bar.finish(dirty=True) diff --git a/tests/test_progressbar_command.py b/tests/test_progressbar_command.py index 6292f6d2..6ce140a4 100644 --- a/tests/test_progressbar_command.py +++ b/tests/test_progressbar_command.py @@ -483,7 +483,7 @@ def write(self, data) -> int: monkeypatch.setattr(main, '_get_output_stream', lambda *args: broken) monkeypatch.setattr(main.sys, 'stdout', broken) monkeypatch.setattr( - main, '_discard_broken_stdout', lambda: discarded.append(True) + main, '_discard_broken_stdout', lambda *args: discarded.append(True) ) main.main([str(file)]) assert discarded == [True] @@ -505,7 +505,7 @@ def fileno(self) -> int: monkeypatch.setattr(main.sys, 'stdout', FakeStdout()) try: - main._discard_broken_stdout() + main._discard_broken_stdout(main.sys.stdout) os.write(1, b'late output') # would raise EPIPE before assert os.fstat(1).st_ino == os.stat(os.devnull).st_ino finally: @@ -528,7 +528,7 @@ def fileno(self) -> int: monkeypatch.setattr(main.sys, 'stdout', PipeStdout()) try: - main._discard_broken_stdout() + main._discard_broken_stdout(main.sys.stdout) os.write(write_end, b'XYZ') assert os.read(read_end, 3) == b'XYZ' # still the pipe, not devnull finally: @@ -536,7 +536,7 @@ def fileno(self) -> int: os.close(write_end) monkeypatch.setattr(main.sys, 'stdout', io.StringIO()) - main._discard_broken_stdout() # no fileno(): nothing to do, no raise + main._discard_broken_stdout(main.sys.stdout) # no fileno(): no raise def test_output_that_is_also_stdin_is_rejected( @@ -576,7 +576,7 @@ def flush(self) -> None: monkeypatch.setattr(main, '_get_output_stream', lambda *args: stream) monkeypatch.setattr(main.sys, 'stdout', stream) monkeypatch.setattr( - main, '_discard_broken_stdout', lambda: discarded.append(True) + main, '_discard_broken_stdout', lambda *args: discarded.append(True) ) main.main([str(file)]) # must not raise, and must not exit non-zero assert discarded == [True] @@ -613,3 +613,60 @@ def test_unreadable_input_leaves_the_output_untouched( secret.chmod(0o600) assert excinfo.value.code == 2 assert output.read_bytes() == b'precious' + + +@pytest.mark.skipif( + not pathlib.Path('/dev/stdout').exists(), reason='needs /dev/stdout' +) +def test_output_path_aliasing_stdout_exits_zero_on_broken_pipe( + tmp_path: pathlib.Path, +) -> None: + # `-o /dev/stdout | head -c1` ended with status 1: the file opened + # for `-o` is not `sys.stdout`, so the broken-pipe handling for the + # reader going away did not apply to it. + import subprocess + import sys + + file = tmp_path / 'data.bin' + file.write_bytes(b'x' * (4 * 1024 * 1024)) + process = subprocess.Popen( + [sys.executable, '-m', 'progressbar', str(file), '-o', '/dev/stdout'], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env={ + **__import__('os').environ, + 'PYTHONPATH': str(pathlib.Path(__file__).parents[1]), + }, + ) + assert process.stdout is not None + assert process.stdout.read(1) == b'x' + process.stdout.close() + _, stderr = process.communicate(timeout=60) + assert process.returncode == 0, stderr.decode(errors='replace') + assert b'Exception ignored' not in stderr + + +def test_output_is_stdout_without_a_descriptor_backed_stdout( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # A stdout without a descriptor (a StringIO under a test runner) can + # alias nothing, so an -o file is never mistaken for it + monkeypatch.setattr(main.sys, 'stdout', io.StringIO()) + assert main._output_is_stdout(io.BytesIO()) is False + + +@pytest.mark.skipif( + not pathlib.Path('/dev/fd/1').exists(), reason='needs /dev/fd' +) +def test_output_is_stdout_for_a_path_that_aliases_descriptor_one( + monkeypatch: pytest.MonkeyPatch, tmp_path: pathlib.Path +) -> None: + class DescriptorOneStdout: + def fileno(self) -> int: + return 1 + + monkeypatch.setattr(main.sys, 'stdout', DescriptorOneStdout()) + with open('/dev/fd/1', 'wb') as alias: # noqa: PTH123 + assert main._output_is_stdout(alias) is True + with open(tmp_path / 'out.bin', 'wb') as regular: # noqa: PTH123 + assert main._output_is_stdout(regular) is False From 0b178cab7b40467ed48b08a2324fa12b113ef31b Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Wed, 28 Oct 2026 01:41:12 +0100 Subject: [PATCH 19/24] Require python-utils 4.1.4 for a non-negative scale_1024 power python-utils before 4.1.4 scaled a value between 0 and 1/1024 to a negative power, and DataSize and format_unit_value() used that power as an index into their prefix tuple, so a fraction of a byte rendered with the largest prefix ('0.1 YiB', '0.1 PiB'). 4.1.4 returns power 0 for those values. The floor moves from 3.8.1 to 4.1.4, the development lock follows, and two tests pin the rendering of a value below one byte. What working code can notice: installing progressbar2 upgrades a python-utils older than 4.1.4. Both packages already require Python 3.10 or newer. --- pyproject.toml | 2 +- tests/test_data_transfer_bar.py | 20 ++++++++++++++++++++ uv.lock | 10 +++++----- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 5797b8ea..8fa00cd3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -107,7 +107,7 @@ classifiers = [ ] requires-python = '>=3.10' -dependencies = ['python-utils >= 3.8.1'] +dependencies = ['python-utils >= 4.1.4'] [project.urls] bugs = 'https://github.com/wolph/python-progressbar/issues' diff --git a/tests/test_data_transfer_bar.py b/tests/test_data_transfer_bar.py index e8f5c577..6b33b0c2 100644 --- a/tests/test_data_transfer_bar.py +++ b/tests/test_data_transfer_bar.py @@ -29,3 +29,23 @@ def test_file_transfer_speed_before_any_data() -> None: output = widget(bar, bar.data()) assert 's/' not in output bar.finish(dirty=True) + + +def test_data_size_below_one_byte_keeps_the_bare_unit() -> None: + # B28: python-utils before 4.1.4 scaled 0.0001 to (0.1024, -1), and + # the negative power picked the largest prefix ('Yi') off the end + # of the tuple. The dependency floor must rule that version out. + widget = progressbar.DataSize() + bar = progressbar.ProgressBar( + max_value=10, widgets=[widget], fd=io.StringIO(), term_width=60 + ) + bar.start() + rendered = widget(bar, {'value': 0.0001}) + bar.finish(dirty=True) + assert rendered.strip() == '0.0 B' + + +def test_format_unit_value_below_one_keeps_the_bare_unit() -> None: + from progressbar.widgets import format_unit_value + + assert format_unit_value(0.0001, unit='B', unit_scale=True) == '0.0 B' diff --git a/uv.lock b/uv.lock index 11eed6ef..2ac3816c 100644 --- a/uv.lock +++ b/uv.lock @@ -1,5 +1,5 @@ version = 1 -revision = 3 +revision = 5 requires-python = ">=3.10" resolution-markers = [ "python_full_version >= '3.15'", @@ -823,7 +823,7 @@ requires-dist = [ { name = "pytest", marker = "extra == 'tests'", specifier = ">=4.6.9" }, { name = "pytest-cov", marker = "extra == 'docs-tests'", specifier = ">=2.6.1" }, { name = "pytest-cov", marker = "extra == 'tests'", specifier = ">=2.6.1" }, - { name = "python-utils", specifier = ">=3.8.1" }, + { name = "python-utils", specifier = ">=4.1.4" }, { name = "pywin32", marker = "sys_platform == 'win32' and extra == 'tests'" }, { name = "speedups", marker = "extra == 'fast'", specifier = ">=2.1.0" }, { name = "sphinx", marker = "extra == 'docs'", specifier = ">=1.8.5" }, @@ -901,14 +901,14 @@ wheels = [ [[package]] name = "python-utils" -version = "3.9.1" +version = "4.1.4" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "typing-extensions" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/13/4c/ef8b7b1046d65c1f18ca31e5235c7d6627ca2b3f389ab1d44a74d22f5cc9/python_utils-3.9.1.tar.gz", hash = "sha256:eb574b4292415eb230f094cbf50ab5ef36e3579b8f09e9f2ba74af70891449a0", size = 35403, upload-time = "2024-11-26T00:38:58.736Z" } +sdist = { url = "https://files.pythonhosted.org/packages/c9/e7/064880019d2f205b837e5c463eb472b36e220d380e5733e49911975f7072/python_utils-4.1.4.tar.gz", hash = "sha256:a8c01f8c0b5a113fa1ab663308bb8a4773afec3cc8f9e6baeb756ac97377f8ce", size = 80130, upload-time = "2026-10-21T01:07:07.458Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/d4/69/31c82567719b34d8f6b41077732589104883771d182a9f4ff3e71430999a/python_utils-3.9.1-py2.py3-none-any.whl", hash = "sha256:0273d7363c7ad4b70999b2791d5ba6b55333d6f7a4e4c8b6b39fb82b5fab4613", size = 32078, upload-time = "2024-11-26T00:38:57.488Z" }, + { url = "https://files.pythonhosted.org/packages/3b/97/4dfe369f4e1eed1f50e7cbfaf5ef5e1fa025e8fa29073290d77d7aea89b1/python_utils-4.1.4-py3-none-any.whl", hash = "sha256:009251f6182f872cd4a74225cdf3636f48ee4596532082bd367797c5924d92bf", size = 49838, upload-time = "2026-10-21T01:07:05.761Z" }, ] [[package]] From 5d80bb15b22db31ebdcd84230ad825271aa0034f Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 29 Oct 2026 04:15:32 +0100 Subject: [PATCH 20/24] Fix stream capture callbacks and closed stdin validation --- CHANGES.rst | 8 ++ progressbar/__main__.py | 9 +- progressbar/utils.py | 14 ++- tests/test_progressbar_command.py | 32 +++++++ tests/test_stream_security.py | 148 ++++++++++++++++++++++++++++++ 5 files changed, 204 insertions(+), 7 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 4e5c6c0c..017bec6e 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -2,6 +2,14 @@ Changelog ========= +Unreleased +---------- + +The ``progressbar`` and ``bar`` commands, and ``python -m progressbar``, +now exit with status 130 after Ctrl-C interrupts a transfer. Previously +they exited with status 0. Scripts that check the exit status can now +distinguish an interrupted transfer from a completed copy. + For the most recent changes to the Python Progressbar please look at the Git releases or the commit log: diff --git a/progressbar/__main__.py b/progressbar/__main__.py index 20ed539e..bc37530a 100644 --- a/progressbar/__main__.py +++ b/progressbar/__main__.py @@ -434,9 +434,14 @@ def _collect_inputs( filesize_available: bool = True for filename in args.input: if filename == '-': - if sys.stdin is None: # started with stdin closed + if sys.stdin is None or sys.stdin.closed: parser.error('stdin is closed, cannot read from -') - inputs.append(sys.stdin if args.line_mode else sys.stdin.buffer) + stdin: IO[typing.Any] = ( + sys.stdin if args.line_mode else sys.stdin.buffer + ) + if stdin.closed: + parser.error('stdin is closed, cannot read from -') + inputs.append(stdin) filesize_available = False continue diff --git a/progressbar/utils.py b/progressbar/utils.py index 5b07fc35..c6c839bb 100644 --- a/progressbar/utils.py +++ b/progressbar/utils.py @@ -379,15 +379,14 @@ def _take(self) -> str: self.buffer.seek(0) self.buffer.truncate(0) self.needs_clear = False - if value: - # Outside the lock: a replacement buffer's flush() may call - # out (log, say), and must not do so while holding a lock - # that write() needs - self.flush() return value def _deliver(self, value: str) -> None: """Write text taken by `_take()` to `target`, then flush it.""" + if value: + # A replacement flush() may log. Call it only during + # delivery, outside both the buffer and transition locks. + self.flush() if value and not self.target.closed: self.target.write(value) # when explicitly flushing, always flush the target as well @@ -570,6 +569,7 @@ def start_capturing(self, bar: _ProgressListener | None = None) -> None: self.capturing += 1 # Never flushes here: the count just went above zero self._apply_capturing() + self.update_capturing() def stop_capturing(self, bar: _ProgressListener | None = None) -> None: """Unregister `bar` and drop the shared capturing refcount. @@ -588,6 +588,10 @@ def stop_capturing(self, bar: _ProgressListener | None = None) -> None: self.capturing = max(self.capturing - 1, 0) pending: list[_Pending] = self._apply_capturing() self._deliver(pending) + # Keep the public subclass hook outside the transition lock. + # The zero transition already owns its drained text, even if + # another capture starts before this callback runs. + self.update_capturing() def update_capturing(self) -> None: # pragma: no cover """Propagate the capturing refcount to the wrapped streams. diff --git a/tests/test_progressbar_command.py b/tests/test_progressbar_command.py index 6ce140a4..e2dc6677 100644 --- a/tests/test_progressbar_command.py +++ b/tests/test_progressbar_command.py @@ -594,6 +594,38 @@ def test_closed_stdin_is_a_usage_error( assert output.read_bytes() == b'precious' +@pytest.mark.parametrize( + ('line_mode', 'closed_part'), + [(False, 'text'), (True, 'text'), (False, 'binary')], +) +def test_closed_stdin_stream_leaves_output_untouched( + tmp_path: pathlib.Path, + monkeypatch: pytest.MonkeyPatch, + line_mode: bool, + closed_part: str, +) -> None: + output: pathlib.Path = tmp_path / 'out.bin' + output.write_bytes(b'precious') + binary: io.BytesIO = io.BytesIO(b'input') + stdin: io.StringIO | io.TextIOWrapper + if closed_part == 'text': + stdin = io.TextIOWrapper(binary) + stdin.close() + else: + # A replacement stdin can expose a closed binary stream while + # its text wrapper remains open. + class SeparateStdin(io.StringIO): + buffer: io.BytesIO = binary + + stdin = SeparateStdin('input') + binary.close() + monkeypatch.setattr(main.sys, 'stdin', stdin) + with pytest.raises(SystemExit) as excinfo: + main.main([*(['-l'] if line_mode else []), '-', '-o', str(output)]) + assert excinfo.value.code == 2 + assert output.read_bytes() == b'precious' + + def test_unreadable_input_leaves_the_output_untouched( tmp_path: pathlib.Path, ) -> None: diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py index 968666d2..1105500c 100644 --- a/tests/test_stream_security.py +++ b/tests/test_stream_security.py @@ -2,7 +2,9 @@ import io import logging +import subprocess import sys +import textwrap import threading import typing from collections.abc import Iterator @@ -862,3 +864,149 @@ def drainer() -> None: assert wrapper.target.getvalue() == 'A|B|flush-log\n' finally: logger.removeHandler(handler) + + +@pytest.mark.parametrize('action', ['flush', 'stop_capturing', 'unwrap_stdout']) +@pytest.mark.parametrize('transition', ['start_capturing', 'stop_capturing']) +def test_buffer_flush_logging_does_not_hold_transition_lock( + action: str, transition: str, +) -> None: + # Isolate a real logging lock cycle so a regression cannot block + # pytest's logging shutdown, even when daemon threads are stuck. + script: str = textwrap.dedent('''\ + import io + import logging + import os + import sys + import threading + from progressbar import utils + + streams: utils.StreamWrapper = utils.StreamWrapper() + target: io.StringIO = io.StringIO() + streams.stdout = streams.original_stdout = target + wrapper: utils.WrappingIO = streams.wrap_stdout() + streams.start_capturing() + entered: threading.Event = threading.Event() + held: threading.Event = threading.Event() + logger: logging.Logger = logging.getLogger('buffer-transition') + logger.setLevel(logging.INFO) + logger.propagate = False + handler: logging.StreamHandler = logging.StreamHandler(io.StringIO()) + logger.addHandler(handler) + failures: list[BaseException] = [] + + class LoggingBuffer(io.StringIO): + fired: bool = False + + def flush(self) -> None: + if not self.fired: + self.fired = True + entered.set() + logger.info('buffer flush') + super().flush() + + wrapper.buffer = LoggingBuffer() + wrapper.write('pending') + + def writer() -> None: + handler.acquire() + try: + held.set() + assert entered.wait(3), 'buffer flush was not called' + getattr(streams, sys.argv[2])() + except BaseException as exception: + failures.append(exception) + finally: + handler.release() + + def drainer() -> None: + try: + assert held.wait(3) + getattr(streams, sys.argv[1])() + except BaseException as exception: + failures.append(exception) + + threads: list[threading.Thread] = [ + threading.Thread(target=writer, daemon=True), + threading.Thread(target=drainer, daemon=True), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join(4) + blocked: bool = any(thread.is_alive() for thread in threads) + delivered: bool = target.getvalue() == 'pending' + print(blocked, delivered, failures, file=sys.__stdout__, flush=True) + os._exit(1 if blocked or failures or not delivered else 0) + ''') + result: subprocess.CompletedProcess[str] = subprocess.run( + [sys.executable, '-c', script, action, transition], + capture_output=True, text=True, timeout=12, check=False, + ) + assert result.returncode == 0, result.stdout + result.stderr + + +def test_capture_transitions_notify_subclass_outside_locks( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[tuple[int, bool]] = [] + unlocked: list[bool] = [] + + class CustomStreams(utils.StreamWrapper): + def update_capturing(self) -> None: + def check_lock() -> None: + acquired: bool = self.lock.acquire(timeout=1) + unlocked.append(acquired) + if acquired: + self.lock.release() + + observer: threading.Thread = threading.Thread(target=check_lock) + observer.start() + observer.join(2) + assert not observer.is_alive() + calls.append((self.capturing, self.stdout.capturing)) + super().update_capturing() + + streams: CustomStreams = CustomStreams() + target: io.StringIO = io.StringIO() + streams.stdout = streams.original_stdout = target + monkeypatch.setattr(sys, 'stdout', target) + wrapper: utils.WrappingIO = streams.wrap_stdout() + streams.start_capturing() + wrapper.write('pending') + streams.stop_capturing() + assert calls == [(1, True), (0, False)] + assert unlocked == [True, True] + assert target.getvalue() == 'pending' + streams.unwrap_stdout() + + +def test_capture_hook_can_start_and_stop_another_capture( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[int] = [] + + class ReentrantStreams(utils.StreamWrapper): + reentered: bool = False + + def update_capturing(self) -> None: + calls.append(self.capturing) + if self.capturing == 0 and not self.reentered: + self.reentered = True + self.start_capturing() + self.stdout.write('nested') + self.stop_capturing() + super().update_capturing() + + streams: ReentrantStreams = ReentrantStreams() + target: io.StringIO = io.StringIO() + streams.stdout = streams.original_stdout = target + monkeypatch.setattr(sys, 'stdout', target) + wrapper: utils.WrappingIO = streams.wrap_stdout() + streams.start_capturing() + wrapper.write('outer') + streams.stop_capturing() + assert calls == [1, 0, 1, 0] + assert streams.capturing == 0 + assert target.getvalue() == 'outernested' + streams.unwrap_stdout() From 1b79d06f9963568716595fc1d35be281a194ec76 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 29 Oct 2026 04:39:17 +0100 Subject: [PATCH 21/24] Notify capture hooks when stream operations fail --- progressbar/utils.py | 53 ++++++++++-- tests/test_stream_security.py | 147 ++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 8 deletions(-) diff --git a/progressbar/utils.py b/progressbar/utils.py index c6c839bb..7f3410d0 100644 --- a/progressbar/utils.py +++ b/progressbar/utils.py @@ -556,11 +556,16 @@ def __init__(self) -> None: def start_capturing(self, bar: _ProgressListener | None = None) -> None: """Turn capturing on for `bar` and bump the shared refcount. + Notify `update_capturing()` outside the locks after propagating + the new state. If propagation fails after the count changes, + still notify the hook and preserve the propagation failure. + Args: bar: Registered as a listener so it's notified (`update()`) when captured output completes a line. Omit to just bump the refcount without listening. """ + failure: BaseException | None = None with self.lock: if bar: # pragma: no branch with self.listeners_lock: @@ -568,16 +573,26 @@ def start_capturing(self, bar: _ProgressListener | None = None) -> None: self.capturing += 1 # Never flushes here: the count just went above zero - self._apply_capturing() - self.update_capturing() + try: + self._apply_capturing() + except BaseException as exception: # noqa: BLE001 + failure = exception + self._notify_capturing(failure) def stop_capturing(self, bar: _ProgressListener | None = None) -> None: """Unregister `bar` and drop the shared capturing refcount. + Notify `update_capturing()` outside the locks after propagating + state and attempting delivery of this stop's owned text. The + hook still runs if propagation or delivery fails. The original + failure takes precedence over an additional hook failure. + Args: bar: The listener to remove, if it was registered via `start_capturing`. """ + pending: list[_Pending] = [] + failure: BaseException | None = None with self.lock: if bar: # pragma: no branch with self.listeners_lock, contextlib.suppress(KeyError): @@ -586,12 +601,27 @@ def stop_capturing(self, bar: _ProgressListener | None = None) -> None: # Never go negative: an unmatched stop would otherwise make # the next start_capturing() a no-op self.capturing = max(self.capturing - 1, 0) - pending: list[_Pending] = self._apply_capturing() - self._deliver(pending) - # Keep the public subclass hook outside the transition lock. - # The zero transition already owns its drained text, even if - # another capture starts before this callback runs. - self.update_capturing() + try: + pending = self._apply_capturing() + except BaseException as exception: # noqa: BLE001 + failure = exception + if failure is None: + try: + self._deliver(pending) + except BaseException as exception: # noqa: BLE001 + failure = exception + self._notify_capturing(failure) + + def _notify_capturing(self, failure: BaseException | None) -> None: + """Notify the public hook without replacing an earlier failure.""" + if failure is None: + self.update_capturing() + else: + try: + self.update_capturing() + finally: + # Keep a hook failure as the original error's context. + raise failure def update_capturing(self) -> None: # pragma: no cover """Propagate the capturing refcount to the wrapped streams. @@ -600,6 +630,13 @@ def update_capturing(self) -> None: # pragma: no cover so whatever's left in the buffer reaches the terminal as soon as the last bar stops capturing, rather than sitting there until something else happens to trigger a flush. + + Start and stop invoke overrides after the state propagation + attempt. Stop also attempts delivery before invoking the hook, + even if that attempt fails. Calling `super()` reconciles the + current state again and can flush an already empty target. + Concurrent transitions can change the state before the hook + runs, so it can observe a later transition's count and flags. """ with self.lock: pending: list[_Pending] = self._apply_capturing() diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py index 1105500c..adbd2fc5 100644 --- a/tests/test_stream_security.py +++ b/tests/test_stream_security.py @@ -1010,3 +1010,150 @@ def update_capturing(self) -> None: assert streams.capturing == 0 assert target.getvalue() == 'outernested' streams.unwrap_stdout() + + +@pytest.mark.parametrize( + 'failure_at', ['write', 'target_flush', 'buffer_flush', 'drain'] +) +@pytest.mark.parametrize('hook_raises', [False, True]) +def test_failed_stop_still_notifies_hook_and_preserves_first_error( + monkeypatch: pytest.MonkeyPatch, + failure_at: str, + hook_raises: bool, +) -> None: + failure: OSError = OSError('original stream failure') + calls: list[int] = [] + unlocked: list[bool] = [] + + class FailingTarget(io.StringIO): + armed: bool = True + + def write(self, text: str) -> int: + if failure_at == 'write' and self.armed: + self.armed = False + raise failure + return super().write(text) + + def flush(self) -> None: + if failure_at == 'target_flush' and self.armed: + self.armed = False + raise failure + super().flush() + + class FailingBuffer(io.StringIO): + armed: bool = True + + def getvalue(self) -> str: + if failure_at == 'drain' and self.armed: + self.armed = False + raise failure + return super().getvalue() + + def flush(self) -> None: + if failure_at == 'buffer_flush' and self.armed: + self.armed = False + raise failure + super().flush() + + class CustomStreams(utils.StreamWrapper): + def update_capturing(self) -> None: + calls.append(self.capturing) + + def check_locks() -> None: + for lock in (self.lock, self.stdout._buffer_box.lock): + acquired: bool = lock.acquire(timeout=1) + unlocked.append(acquired) + if acquired: + lock.release() + + observer: threading.Thread = threading.Thread(target=check_locks) + observer.start() + observer.join(3) + assert not observer.is_alive() + super().update_capturing() + if self.capturing == 0 and hook_raises: + raise RuntimeError('secondary callback failure') + + streams: CustomStreams = CustomStreams() + out: FailingTarget = FailingTarget() + err: io.StringIO = io.StringIO() + streams.stdout = streams.original_stdout = out + streams.stderr = streams.original_stderr = err + monkeypatch.setattr(sys, 'stdout', out) + monkeypatch.setattr(sys, 'stderr', err) + stdout: utils.WrappingIO = streams.wrap_stdout() + stderr: utils.WrappingIO = streams.wrap_stderr() + streams.start_capturing() + stdout.buffer = FailingBuffer() + stdout.write('spam') + stderr.write('eggs') + with pytest.raises(OSError) as excinfo: + streams.stop_capturing() + assert excinfo.value is failure + if hook_raises: + assert isinstance(failure.__context__, RuntimeError) + assert str(failure.__context__) == 'secondary callback failure' + assert calls == [1, 0] + assert unlocked == [True, True, True, True] + assert streams.capturing == 0 + assert stdout.capturing is False and stderr.capturing is False + assert err.getvalue() == 'eggs' + assert out.getvalue() == ( + 'spam' if failure_at in ('target_flush', 'drain') else '' + ) + streams.unwrap_stdout() + streams.unwrap_stderr() + + +def test_failed_start_state_propagation_still_notifies_hook( + monkeypatch: pytest.MonkeyPatch, +) -> None: + failure: OSError = OSError('capture flag failed') + calls: list[int] = [] + unlocked: list[bool] = [] + + class FailingWrapper(utils.WrappingIO): + armed: bool = False + + def __setattr__(self, name: str, value: typing.Any) -> None: + if name == 'capturing' and self.armed: + self.armed = False + raise failure + super().__setattr__(name, value) + + class CustomStreams(utils.StreamWrapper): + def update_capturing(self) -> None: + calls.append(self.capturing) + + def check_lock() -> None: + acquired: bool = self.lock.acquire(timeout=1) + unlocked.append(acquired) + if acquired: + self.lock.release() + + observer: threading.Thread = threading.Thread(target=check_lock) + observer.start() + observer.join(2) + assert not observer.is_alive() + super().update_capturing() + raise RuntimeError('secondary callback failure') + + streams: CustomStreams = CustomStreams() + target: io.StringIO = io.StringIO() + streams.stdout = streams.original_stdout = target + monkeypatch.setattr(sys, 'stdout', target) + streams.wrap_stdout() + wrapper: FailingWrapper = FailingWrapper(target) + streams.stdout = wrapper + monkeypatch.setattr(sys, 'stdout', wrapper) + wrapper.armed = True + with pytest.raises(OSError) as excinfo: + streams.start_capturing() + assert excinfo.value is failure + assert isinstance(failure.__context__, RuntimeError) + assert str(failure.__context__) == 'secondary callback failure' + assert calls == [1] + assert unlocked == [True] + assert streams.capturing == 1 + assert wrapper.capturing is True + streams.unwrap_stdout() From 61e6cf4fee889f371647bf6764aea9920d5ae023 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 08:26:11 +0000 Subject: [PATCH 22/24] [autofix.ci] apply automated fixes --- tests/test_stream_security.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py index adbd2fc5..d4859094 100644 --- a/tests/test_stream_security.py +++ b/tests/test_stream_security.py @@ -866,14 +866,17 @@ def drainer() -> None: logger.removeHandler(handler) -@pytest.mark.parametrize('action', ['flush', 'stop_capturing', 'unwrap_stdout']) +@pytest.mark.parametrize( + 'action', ['flush', 'stop_capturing', 'unwrap_stdout'] +) @pytest.mark.parametrize('transition', ['start_capturing', 'stop_capturing']) def test_buffer_flush_logging_does_not_hold_transition_lock( - action: str, transition: str, + action: str, + transition: str, ) -> None: # Isolate a real logging lock cycle so a regression cannot block # pytest's logging shutdown, even when daemon threads are stuck. - script: str = textwrap.dedent('''\ + script: str = textwrap.dedent("""\ import io import logging import os @@ -938,10 +941,13 @@ def drainer() -> None: delivered: bool = target.getvalue() == 'pending' print(blocked, delivered, failures, file=sys.__stdout__, flush=True) os._exit(1 if blocked or failures or not delivered else 0) - ''') + """) result: subprocess.CompletedProcess[str] = subprocess.run( [sys.executable, '-c', script, action, transition], - capture_output=True, text=True, timeout=12, check=False, + capture_output=True, + text=True, + timeout=12, + check=False, ) assert result.returncode == 0, result.stdout + result.stderr From aadba2843c5f76f071ee787ad94f2e57d10cb8dc Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 29 Oct 2026 11:03:06 +0100 Subject: [PATCH 23/24] Fix remaining stream, formatter and MultiBar regressions --- CHANGES.rst | 18 ++ CONTRIBUTING.rst | 12 +- progressbar/_identity.py | 79 +++++++ progressbar/multi.py | 210 ++++++++++++----- progressbar/utils.py | 115 ++++++--- progressbar/widgets.py | 53 +++-- pyproject.toml | 1 + ruff.toml | 4 + tests/test_format_security.py | 85 +++++++ tests/test_multibar.py | 423 ++++++++++++++++++++++++++++++++++ tests/test_stream_security.py | 240 ++++++++++++++++--- uv.lock | 2 + 12 files changed, 1096 insertions(+), 146 deletions(-) create mode 100644 progressbar/_identity.py diff --git a/CHANGES.rst b/CHANGES.rst index 017bec6e..80b028be 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -10,6 +10,24 @@ now exit with status 130 after Ctrl-C interrupts a transfer. Previously they exited with status 0. Scripts that check the exit status can now distinguish an interrupted transfer from a completed copy. +Captured output now survives a drain failure on the other stream, and +unwrapping restores the original stream even when its capture buffer is +closed. Built-in ``StringIO`` capture writes and drains remain atomic. +Caller-replaced buffers keep their method overrides, which run without +stream locks. As in 4.6.0, callers synchronise concurrent writes and drains +on those custom buffers. + +Specialised format templates retain their escaping and result types. +Their field paths, including nested format specifications, are checked +before calling the application's formatter. + +MultiBar expiry keeps a replacement registered under the same label. +Assignment, update, in-place union and ordinary removal methods share its +membership guard. Lifecycle tracking uses bar identity without calling +application hash or equality methods, and the guard resets after fork. +Direct calls to ``dict`` base-class mutation methods +bypass that guard and are not supported during concurrent rendering. + For the most recent changes to the Python Progressbar please look at the Git releases or the commit log: diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 5477d533..8a900ccf 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -118,14 +118,18 @@ Ready to contribute? Here's how to set up `python-progressbar` for local develop Now you can make your changes locally. -5. When you're done making changes, check that your changes pass the - linter and the tests, including other Python versions with tox:: +5. Before committing, format the changed Python files, then check the + whole tree and run the tests, including other Python versions with tox:: - $ uv run ruff check progressbar tests + $ uvx ruff@latest format path/to/changed.py + $ uvx ruff@latest check . + $ uvx ruff@latest format --check . $ uv run pytest $ uv run tox - The ``uv sync`` from step 3 already installed all of these. + Stage the formatted changes before committing. A checkout-local + pre-commit gate, when installed, checks the staged snapshot and leaves + partially staged files untouched. 6. Commit your changes and push your branch to GitHub with `git-flow-avh`_:: diff --git a/progressbar/_identity.py b/progressbar/_identity.py new file mode 100644 index 00000000..b7245c94 --- /dev/null +++ b/progressbar/_identity.py @@ -0,0 +1,79 @@ +"""Private identity-based tracking that never calls application hash hooks.""" + +from __future__ import annotations + +import collections.abc +import typing + +_Key = typing.TypeVar('_Key') +_Value = typing.TypeVar('_Value') + + +class IdentityMap(collections.abc.MutableMapping[_Key, _Value]): + """Track bars by identity without calling their hash or equality hooks.""" + + def __init__(self) -> None: + self._items: dict[int, tuple[_Key, _Value]] = {} + + def __getstate__(self) -> list[tuple[_Key, _Value]]: + """Serialise retained objects without their process-local ids.""" + return list(self._items.values()) + + def __setstate__(self, state: list[tuple[_Key, _Value]]) -> None: + """Rebuild ids after pickle or deepcopy restores the objects.""" + self._items = {id(key): (key, value) for key, value in state} + + def __getitem__(self, key: _Key) -> _Value: + """Return the value belonging to this exact bar.""" + return self._items[id(key)][1] + + def __setitem__(self, key: _Key, value: _Value) -> None: + """Retain the bar alongside its value to prevent id reuse.""" + self._items[id(key)] = (key, value) + + def __delitem__(self, key: _Key) -> None: + """Remove this exact bar's entry.""" + del self._items[id(key)] + + def __iter__(self) -> collections.abc.Iterator[_Key]: + """Iterate the retained bars.""" + return (item[0] for item in self._items.values()) + + def __len__(self) -> int: + """Return the number of tracked bars.""" + return len(self._items) + + +class IdentitySet(collections.abc.MutableSet[_Key]): + """Remember labelled bars without calling application hash hooks.""" + + def __init__(self) -> None: + self._items: dict[int, _Key] = {} + + def __getstate__(self) -> list[_Key]: + """Serialise retained objects without their process-local ids.""" + return list(self._items.values()) + + def __setstate__(self, state: list[_Key]) -> None: + """Rebuild ids after pickle or deepcopy restores the objects.""" + self._items = {id(value): value for value in state} + + def __contains__(self, value: object) -> bool: + """Check whether this exact object is remembered.""" + return id(value) in self._items + + def __iter__(self) -> collections.abc.Iterator[_Key]: + """Iterate remembered bars.""" + return iter(self._items.values()) + + def __len__(self) -> int: + """Return the number of remembered bars.""" + return len(self._items) + + def add(self, value: _Key) -> None: + """Retain a labelled bar by identity.""" + self._items[id(value)] = value + + def discard(self, value: _Key) -> None: + """Forget a labelled bar if it was remembered.""" + self._items.pop(id(value), None) diff --git a/progressbar/multi.py b/progressbar/multi.py index d113500b..6259b82d 100644 --- a/progressbar/multi.py +++ b/progressbar/multi.py @@ -10,7 +10,6 @@ from __future__ import annotations import collections.abc -import contextlib import enum import importlib import io @@ -26,7 +25,7 @@ import python_utils -from . import bar, terminal +from . import _identity, bar, terminal, utils from .terminal import stream # MultiBar renders full (widget) progress bars from background threads. Warm @@ -164,8 +163,9 @@ class MultiBar(dict[str, bar.ProgressBar]): sort_keyfunc: SortKeyFunc _previous_output: list[str] - _finished_at: dict[bar.ProgressBar, float] - _labeled: set[bar.ProgressBar] + _finished_at: _identity.IdentityMap[bar.ProgressBar, float] + _labeled: _identity.IdentitySet[bar.ProgressBar] + _membership_box: utils._SharedLock # pyright: ignore[reportPrivateUsage] _print_lock: threading.RLock _thread: threading.Thread | None _thread_finished: threading.Event @@ -223,10 +223,13 @@ def __init__( self.sort_keyfunc = sort_keyfunc self.sort_reverse = sort_reverse - self._labeled = set() - self._finished_at = {} + self._labeled = _identity.IdentitySet[bar.ProgressBar]() + self._finished_at = _identity.IdentityMap() self._previous_output = [] self._buffer = io.StringIO() + self._membership_box = utils._SharedLock( # pyright: ignore[reportPrivateUsage] + threading.RLock + ) self._print_lock = threading.RLock() self._thread = None self._thread_finished = threading.Event() @@ -248,6 +251,11 @@ def __init__( for key, progress in bar_items: self[key] = progress + @property + def _membership_lock(self) -> threading.RLock: + """Return the membership guard, reset automatically after fork.""" + return self._membership_box.lock + def __setitem__(self, key: str, bar: bar.ProgressBar) -> None: """Add `bar` to the multibar, rebinding it to draw through us. @@ -277,57 +285,138 @@ def __setitem__(self, key: str, bar: bar.ProgressBar) -> None: key: The label and dict key to add/update `bar` under. bar: The progressbar to add, mutated as described above. """ - if bar.label != key or not key: # pragma: no branch - bar.label = key - + self._prepare_bar(key, bar) + with self._membership_lock: + previous: bar.ProgressBar | None = super().get(key) + super().__setitem__(key, bar) + if previous is not bar: + self._forget_bar(previous) + + def _prepare_bar(self, key: str, progress: bar.ProgressBar) -> None: + """Configure a bar outside the membership guard.""" + if progress.label != key or not key: # pragma: no branch + progress.label = key if not ( - isinstance(bar.fd, stream.LastLineStream) - and bar.fd.stream is self.fd + isinstance(progress.fd, stream.LastLineStream) + and progress.fd.stream is self.fd ): - bar.fd = stream.LastLineStream(self.fd) - - bar.paused = True - # `mypy` rejects assigning to a method, hence the ignore. - bar.print = self.print # type: ignore - - # Just in case someone is using a progressbar with a custom - # constructor and forgot to call the super constructor - if bar.index == -1: - bar.index = next( - bar._index_counter # pyright: ignore[reportPrivateUsage] + progress.fd = stream.LastLineStream(self.fd) + progress.paused = True + progress.print = self.print # type: ignore + if progress.index == -1: + progress.index = next( + progress._index_counter # pyright: ignore[reportPrivateUsage] ) - super().__setitem__(key, bar) + def _forget_bar(self, progress: bar.ProgressBar | None) -> None: + """Discard tracking for a removed member under the guard.""" + if progress is not None: + self._finished_at.pop(progress, None) + self._labeled.discard(progress) def __delitem__(self, key: str) -> None: - """Remove a progressbar from the multibar.""" - bar_: bar.ProgressBar = self.pop(key) - self._finished_at.pop(bar_, None) - self._labeled.discard(bar_) + """Remove a progressbar and its tracking.""" + self.pop(key) def __getitem__(self, key: str) -> bar.ProgressBar: """Get (and create if needed) a progressbar from the multibar.""" - try: - return super().__getitem__(key) - except KeyError: - progress = bar.ProgressBar(**self.progressbar_kwargs) + with self._membership_lock: + try: + return super().__getitem__(key) + except KeyError: + pass + progress: bar.ProgressBar = bar.ProgressBar(**self.progressbar_kwargs) + return self.setdefault(key, progress) + + def update(self, *args: typing.Any, **kwargs: bar.ProgressBar) -> None: + """Add mapping or iterable entries through normal bar setup.""" + entries: dict[str, bar.ProgressBar] = dict(*args, **kwargs) + for key, progress in entries.items(): self[key] = progress - return progress - def _label_bar(self, bar: bar.ProgressBar) -> None: - """Insert the label widget(s) into `bar.widgets`, once per bar.""" - if bar in self._labeled: # pragma: no branch - return + def __ior__(self, other: typing.Any) -> MultiBar: + """Update in place using the same membership guard as assignment.""" + self.update(other) + return self - assert bar.widgets, 'Cannot prepend label to empty progressbar' + @typing.overload + def setdefault( + self, key: str, default: bar.ProgressBar + ) -> bar.ProgressBar: ... + + @typing.overload + def setdefault( + self, key: str, default: None = None + ) -> bar.ProgressBar | None: ... + + def setdefault( + self, key: str, default: bar.ProgressBar | None = None + ) -> bar.ProgressBar | None: + """Insert a missing member without replacing a concurrent one.""" + with self._membership_lock: + if super().__contains__(key): + return super().__getitem__(key) + if default is not None: + self._prepare_bar(key, default) + with self._membership_lock: + members: dict[str, bar.ProgressBar | None] = typing.cast( + dict[str, bar.ProgressBar | None], self + ) + return dict[str, bar.ProgressBar | None].setdefault( + members, key, default + ) - if self.prepend_label: # pragma: no branch - self._labeled.add(bar) - bar.widgets.insert(0, self.label_format.format(label=bar.label)) + def pop(self, key: str, *args: typing.Any) -> typing.Any: + """Remove a member and discard its lifecycle tracking.""" + with self._membership_lock: + present: bool = super().__contains__(key) + progress: typing.Any = super().pop(key, *args) + if present: + self._forget_bar(progress) + return progress + def popitem(self) -> tuple[str, bar.ProgressBar]: + """Remove the newest member and its tracking.""" + with self._membership_lock: + key: str + progress: bar.ProgressBar + key, progress = super().popitem() + self._forget_bar(progress) + return key, progress + + def clear(self) -> None: + """Remove all members and their tracking.""" + removed: list[bar.ProgressBar] + with self._membership_lock: + removed = list(super().values()) + removed.extend(self._finished_at) + removed.extend(self._labeled) + super().clear() + self._finished_at.clear() + self._labeled.clear() + # Release the last references after leaving the membership guard. + del removed + + def _label_bar( + self, bar: bar.ProgressBar, *, require_member: bool = False + ) -> bool: + """Reserve a bar's label under the guard, then format it unlocked.""" + label: str = bar.label + widgets: collections.abc.MutableSequence[typing.Any] = bar.widgets + assert widgets, 'Cannot prepend label to empty progressbar' + with self._membership_lock: + if require_member and super().get(label) is not bar: + return False + if bar in self._labeled: # pragma: no branch + return True + if self.prepend_label or self.append_label: + self._labeled.add(bar) + + if self.prepend_label: # pragma: no branch + widgets.insert(0, self.label_format.format(label=label)) if self.append_label: # pragma: no branch - self._labeled.add(bar) - bar.widgets.append(self.label_format.format(label=bar.label)) + widgets.append(self.label_format.format(label=label)) + return True def render(self, flush: bool = True, force: bool = False) -> None: """Redraw every bar, only touching lines that actually changed. @@ -419,7 +508,8 @@ def _render_bar( def update( force: bool = True, write: bool = True ) -> str: # pragma: no cover - self._label_bar(bar_) + if not self._label_bar(bar_, require_member=True): + return '' bar_.update(force=force) if write: return typing.cast(stream.LastLineStream, bar_.fd).line @@ -457,19 +547,16 @@ def _render_finished_bar( Returns: Zero lines (dropped or hidden) or one rendered line. """ - if self.get(bar_.label) is not bar_: - # Removed between the render snapshot and now (a task - # finished on another thread): nothing to draw, and nothing - # to remember about it - return - finished_at: float | None = self._finished_at.get(bar_) - if finished_at is None: - finished_at = self._finished_at.setdefault(bar_, now) - if self.get(bar_.label) is not bar_: # pragma: no cover - # Removed while we stamped it: undo the stamp - self._finished_at.pop(bar_, None) + label: str = bar_.label + with self._membership_lock: + if super().get(label) is not bar_: return - # Force update to get the finished format + finished_at: float | None = self._finished_at.get(bar_) + first_finish: bool = finished_at is None + if finished_at is None: + finished_at = self._finished_at.setdefault(bar_, now) + if first_finish: + # Widgets and targets may call back into membership methods. update(write=False) if ( @@ -477,8 +564,10 @@ def _render_finished_bar( and expired is not None and expired >= finished_at ): - with contextlib.suppress(KeyError): # removed concurrently - del self[bar_.label] + with self._membership_lock: + if super().get(label) is bar_: + super().__delitem__(label) + self._forget_bar(bar_) return if not self.show_finished: @@ -674,10 +763,11 @@ def get_sorted_bars(self) -> list[bar.ProgressBar]: The bars sorted by `sort_keyfunc`, reversed if `sort_reverse`. The values are copied into a list first so a concurrent `__setitem__`/`__delitem__` from another - thread (the multibar is a plain `dict`, not a thread-safe - one) can't mutate it out from under the sort. + thread cannot mutate it during the snapshot. Sorting runs + after releasing the membership guard. """ - bars = list(self.values()) + with self._membership_lock: + bars: list[bar.ProgressBar] = list(super().values()) return sorted(bars, key=self.sort_keyfunc, reverse=self.sort_reverse) def __enter__(self) -> MultiBar: diff --git a/progressbar/utils.py b/progressbar/utils.py index 7f3410d0..58aad128 100644 --- a/progressbar/utils.py +++ b/progressbar/utils.py @@ -174,7 +174,8 @@ def len_color(value: types.StringTypes) -> int: #: One installed wrapper's drained text, waiting to be written to the #: original stream once the transition lock is released. -_Pending = tuple[str, 'WrappingIO', str] +_Drain = str | BaseException | None +_Pending = tuple[str, 'WrappingIO', _Drain] if typing.TYPE_CHECKING: _Lock = threading.Lock @@ -255,7 +256,7 @@ class WrappingIO: #: interleaving on `buffer`: a write between the read and the #: truncate of a drain was lost. Held around the `io.StringIO` #: write, getvalue, seek and truncate calls only, never while - #: calling a listener, `target` or the buffer's `flush()`. + #: calling a listener, `target` or a custom buffer method. _buffer_box: _SharedLock = _SharedLock(threading.RLock) needs_clear: bool = False @@ -324,7 +325,11 @@ def write(self, value: str) -> int: """ ret = 0 if self.capturing: - with self._buffer_box.lock: + with ( + self._buffer_box.lock + if self._native_buffer() + else contextlib.nullcontext() + ): ret += self.buffer.write(value) if '\n' in value: # pragma: no branch self.needs_clear = True @@ -366,11 +371,15 @@ def _flush(self) -> None: def _take(self) -> str: """Drain and return the buffered text without touching `target`. - `StreamWrapper` calls this under its transition lock, so two - overlapping flushes cannot both pick up the same text, and - delivers the result after releasing it. + Native StringIO drains hold the buffer lock. `StreamWrapper` + also captures them under its transition lock. Custom buffer + methods run unlocked and retain caller-owned synchronisation. """ - with self._buffer_box.lock: + with ( + self._buffer_box.lock + if self._native_buffer() + else contextlib.nullcontext() + ): value: str = self.buffer.getvalue() if value: # Clear the buffer before writing so a failed write @@ -381,6 +390,36 @@ def _take(self) -> str: self.needs_clear = False return value + def _native_buffer(self) -> bool: + """Recognise built-in operations, including instance shadowing. + + Custom buffers keep their original method dispatch. Their callers + own synchronisation of concurrent writes and drains, as in 4.6.0. + """ + return type(self.buffer) is io.StringIO and not any( + name in vars(self.buffer) + for name in ('write', 'getvalue', 'seek', 'truncate', 'flush') + ) + + def _capture(self) -> _Drain: + """Take native text or its failure now, defer custom code.""" + if ( + not self._native_buffer() + or type(self)._take is not WrappingIO._take + or '_take' in vars(self) + ): + return None + try: + return self._take() + except BaseException as exception: # noqa: BLE001 + return exception + + def _deliver_capture(self, captured: _Drain) -> None: + """Drain deferred custom text and deliver outside stream locks.""" + if isinstance(captured, BaseException): + raise captured + self._deliver(self._take() if captured is None else captured) + def _deliver(self, value: str) -> None: """Write text taken by `_take()` to `target`, then flush it.""" if value: @@ -665,12 +704,12 @@ def _apply_capturing(self) -> list[_Pending]: return self._take() def _take(self) -> list[_Pending]: - """Drain both installed wrappers' buffers, under `lock`.""" + """Capture native text or failures and defer custom drains.""" pending: list[_Pending] = [] if self.wrapped_stdout and isinstance(self.stdout, WrappingIO): - pending.append(('stdout', self.stdout, self.stdout._take())) + pending.append(('stdout', self.stdout, self.stdout._capture())) if self.wrapped_stderr and isinstance(self.stderr, WrappingIO): - pending.append(('stderr', self.stderr, self.stderr._take())) + pending.append(('stderr', self.stderr, self.stderr._capture())) return pending def _deliver(self, pending: list[_Pending]) -> None: @@ -696,11 +735,13 @@ def _deliver(self, pending: list[_Pending]) -> None: ) if failure is None: failure = exception + elif exception is not None and exception is not failure: + failure.__context__ = exception if failure is not None: raise failure def _deliver_guarded( - self, name: str, wrapper: WrappingIO, text: str + self, name: str, wrapper: WrappingIO, text: _Drain ) -> BaseException | None: """Deliver one wrapper's text, returning what it raised.""" try: @@ -709,10 +750,12 @@ def _deliver_guarded( return exception return None - def _deliver_one(self, name: str, wrapper: WrappingIO, text: str) -> None: + def _deliver_one( + self, name: str, wrapper: WrappingIO, text: _Drain + ) -> None: """Deliver one wrapper's text, disabling its redirection on failure.""" try: - wrapper._deliver(text) + wrapper._deliver_capture(text) except io.UnsupportedOperation: setattr(self, f'wrapped_{name}', 0) logger.warning( @@ -954,7 +997,7 @@ def unwrap(self, stdout: bool = False, stderr: bool = False) -> None: self.unwrap_stderr() @staticmethod - def _drop(dropped: object) -> tuple[WrappingIO, str] | None: + def _drop(dropped: object) -> tuple[WrappingIO, _Drain] | None: """Stop buffering in a dropped wrapper and take what it holds. Runs under `lock`; the text goes to `_deliver_dropped` after @@ -963,10 +1006,10 @@ def _drop(dropped: object) -> tuple[WrappingIO, str] | None: if not isinstance(dropped, WrappingIO): return None dropped.capturing = False - return dropped, dropped._take() + return dropped, dropped._capture() @staticmethod - def _deliver_dropped(taken: tuple[WrappingIO, str] | None) -> None: + def _deliver_dropped(taken: tuple[WrappingIO, _Drain] | None) -> None: """Deliver the text `_drop` took, outside `lock`. A stale reference to the wrapper must not swallow output. The @@ -979,7 +1022,7 @@ def _deliver_dropped(taken: tuple[WrappingIO, str] | None) -> None: return dropped, text = taken try: - dropped._deliver(text) + dropped._deliver_capture(text) except Exception: logger.debug( 'Could not deliver buffered output on unwrap', @@ -1001,13 +1044,15 @@ def unwrap_stdout(self) -> None: self.wrapped_stdout -= 1 return - taken = self._drop(self.stdout) - # Also reset our own reference so needs_clear() and - # update_capturing() don't act on a stale wrapper - self.stdout = sys.stdout = self.original_stdout - self.wrapped_stdout = 0 - if not self.wrapped_stderr: - self.unwrap_excepthook() + try: + taken = self._drop(self.stdout) + finally: + # Also reset our own reference so needs_clear() and + # update_capturing() don't act on a stale wrapper + self.stdout = sys.stdout = self.original_stdout + self.wrapped_stdout = 0 + if not self.wrapped_stderr: + self.unwrap_excepthook() # Deliver after releasing `lock`: this writes into the original # stream and may log, see `_apply_capturing` self._deliver_dropped(taken) @@ -1025,13 +1070,15 @@ def unwrap_stderr(self) -> None: self.wrapped_stderr -= 1 return - taken = self._drop(self.stderr) - # Also reset our own reference so needs_clear() and - # update_capturing() don't act on a stale wrapper - self.stderr = sys.stderr = self.original_stderr - self.wrapped_stderr = 0 - if not self.wrapped_stdout: - self.unwrap_excepthook() + try: + taken = self._drop(self.stderr) + finally: + # Also reset our own reference so needs_clear() and + # update_capturing() don't act on a stale wrapper + self.stderr = sys.stderr = self.original_stderr + self.wrapped_stderr = 0 + if not self.wrapped_stdout: + self.unwrap_excepthook() # Deliver after releasing `lock`, see `unwrap_stdout` self._deliver_dropped(taken) @@ -1053,9 +1100,9 @@ def needs_clear(self) -> bool: # pragma: no cover def flush(self) -> None: """Flush buffered captured output on both wrapped streams. - The buffers are drained under `lock` and written to the - original streams after releasing it, see `_deliver` for what - happens when a stream cannot take the text. + Native buffers are drained under `lock`. Custom drains and + writes to original streams run after releasing it. See + `_deliver` for what happens when a stream cannot take the text. """ with self.lock: pending: list[_Pending] = self._take() diff --git a/progressbar/widgets.py b/progressbar/widgets.py index 5fa1a4e9..2bdf78bb 100644 --- a/progressbar/widgets.py +++ b/progressbar/widgets.py @@ -65,6 +65,11 @@ def get_field( args: typing.Sequence[typing.Any], kwargs: typing.Mapping[str, typing.Any], ) -> tuple[typing.Any, str]: + self.validate_field(field_name) + return super().get_field(field_name, args, kwargs) + + def validate_field(self, field_name: str) -> None: + """Check every attribute before any value is looked up.""" rest: typing.Iterable[tuple[bool, typing.Any]] _first, rest = _string.formatter_field_name_split(field_name) for is_attr, name in rest: @@ -74,7 +79,23 @@ def get_field( raise ValueError( f'attribute {name!r} is not allowed in a format template', ) - return super().get_field(field_name, args, kwargs) + + def validate(self, template: str, depth: int = 2) -> None: + """Check specialised templates before handing them to their owner.""" + if depth < 0: + raise ValueError('Max string recursion exceeded') + for _, field, spec, conversion in self.parse(template): + if field is not None: + self.validate_field(field) + if conversion is not None and conversion not in ( + 's', + 'r', + 'a', + ): + raise ValueError( + f'Unknown conversion specifier {conversion}' + ) + self.validate(spec or '', depth - 1) _SAFE_FORMATTER: _SafeFormatter = _SafeFormatter() @@ -84,22 +105,24 @@ def _safe_format( template: object, data: typing.Mapping[str, typing.Any], ) -> str: - """Render `template` with `data` through the safe formatter. + """Render safe field paths while preserving specialised formatting. - `string.Formatter` needs a real `str`, while `str.format` accepted - anything with a `format()` method. A translation proxy (the - `gettext_lazy` shape) is therefore rendered through `str()` first, - so it keeps working. Anything else that is not text (None, a list, - a number) is a `TypeError`, as it was before. + Application-owned format methods retain their escaping and result + types. Field paths in the underlying string or proxy text are + validated before delegation. Application code in format methods + and proxies remains trusted. """ - if not isinstance(template, str): - if not callable(getattr(template, 'format', None)): - raise TypeError( - 'format template must be a str, got ' - f'{type(template).__name__}', - ) - template = str(template) - return _SAFE_FORMATTER.vformat(template, (), data) + if type(template) is str: + return _SAFE_FORMATTER.vformat(template, (), data) + formatter: typing.Any = getattr(template, 'format', None) + if not callable(formatter): + raise TypeError( + f'format template must be a str, got {type(template).__name__}', + ) + _SAFE_FORMATTER.validate( + str.__str__(template) if isinstance(template, str) else str(template) + ) + return typing.cast(str, formatter(**data)) MAX_DATE = datetime.date.max diff --git a/pyproject.toml b/pyproject.toml index 8fa00cd3..544e3d47 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -133,6 +133,7 @@ docs = [ ] tests = [ 'dill>=0.3.6', + 'markupsafe>=2.0', 'freezegun>=0.3.11', 'pytest-cov>=2.6.1', 'pytest>=4.6.9', diff --git a/ruff.toml b/ruff.toml index 7a95ec7d..7b5dda6c 100644 --- a/ruff.toml +++ b/ruff.toml @@ -121,3 +121,7 @@ max-line-length = 79 [lint.flake8-pytest-style] mark-parentheses = true + +[lint.flake8-import-conventions.aliases] +# Preserve the established module spelling, including public annotations. +datetime = 'datetime' diff --git a/tests/test_format_security.py b/tests/test_format_security.py index de05ccfa..ca48af2c 100644 --- a/tests/test_format_security.py +++ b/tests/test_format_security.py @@ -287,3 +287,88 @@ def test_frame_walks_are_refused_like_dunders() -> None: label = progressbar.FormatLabel(template, new_style=True) with pytest.raises(ValueError, match='not allowed'): label(bar, data) + + +def test_specialised_template_preserves_markup_escaping_and_type() -> None: + from markupsafe import Markup + + bar, data = _label_data() + data['value'] = '' + label: progressbar.FormatLabel = progressbar.FormatLabel( + Markup('{value}'), new_style=True + ) + result: str = label(bar, data) + assert result == '<tag>' + assert isinstance(result, Markup) + + +@pytest.mark.parametrize( + 'template', + [ + '{value.__class__!s}', + '{value:{width.__class__}}', + '{value:{width.f_globals}}', + '{value!z}', + '{value:{width:{precision}}}', + ], +) +def test_specialised_template_validates_before_delegation( + template: str, +) -> None: + calls: list[str] = [] + + class Template(str): + def format(self, *args: object, **kwargs: object) -> str: + calls.append(self) + return 'specialised' + + bar, data = _label_data() + label: progressbar.FormatLabel = progressbar.FormatLabel( + Template(template), new_style=True + ) + with pytest.raises(ValueError): + label(bar, data) + assert calls == [] + + +def test_specialised_proxy_preserves_format_method() -> None: + class Template(_LazyStr): + def format(self, *args: object, **kwargs: object) -> str: + return 'specialised:' + super().format(*args, **kwargs) + + bar, data = _label_data() + label: progressbar.FormatLabel = progressbar.FormatLabel( + Template('{value}'), new_style=True + ) # type: ignore[arg-type] + assert label(bar, data) == 'specialised:3' + + +def test_specialised_str_cannot_hide_field_paths_in_str_override() -> None: + calls: list[str] = [] + + class Template(str): + def __str__(self) -> str: + return '{value}' + + def format(self, *args: object, **kwargs: object) -> str: + calls.append('called') + return super().format(*args, **kwargs) + + bar, data = _label_data() + label: progressbar.FormatLabel = progressbar.FormatLabel( + Template('{value.__class__}'), new_style=True + ) + with pytest.raises(ValueError, match='__class__'): + label(bar, data) + assert calls == [] + + +def test_specialised_template_allows_literal_braces_and_nested_width() -> None: + from markupsafe import Markup + + bar, data = _label_data() + data['width'] = 3 + label: progressbar.FormatLabel = progressbar.FormatLabel( + Markup('{{literal}} {value:0{width}d}'), new_style=True + ) + assert label(bar, data) == '{literal} 003' diff --git a/tests/test_multibar.py b/tests/test_multibar.py index bec2c712..d32fd760 100644 --- a/tests/test_multibar.py +++ b/tests/test_multibar.py @@ -595,3 +595,426 @@ def test_render_of_a_bar_removed_mid_frame_does_not_leak() -> None: ) assert lines == [] assert bar_ not in multibar._finished_at + + +@pytest.mark.parametrize('mutation', ['assign', 'update', 'ior']) +def test_expiry_keeps_concurrent_replacement(mutation: str) -> None: + import inspect + import sys + import types + import typing + + multibar: progressbar.MultiBar = progressbar.MultiBar( + fd=io.StringIO(), remove_finished=1 + ) + old: progressbar.ProgressBar = multibar['task'] + replacement: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO() + ) + multibar._finished_at[old] = 0 + multibar._labeled.add(old) + reached: threading.Event = threading.Event() + resume: threading.Event = threading.Event() + errors: list[BaseException] = [] + lines, first = inspect.getsourcelines(multibar._render_finished_bar) + pause_line: int = first + next( + i for i, line in enumerate(lines) if 'self.remove_finished' in line + ) + + def trace( + frame: types.FrameType, event: str, arg: typing.Any + ) -> typing.Any: + if ( + event == 'line' + and frame.f_code is multibar._render_finished_bar.__func__.__code__ + and frame.f_lineno == pause_line + ): + reached.set() + assert resume.wait(3) + return trace + + def render() -> None: + sys.settrace(trace) + try: + list( + multibar._render_finished_bar(old, 10, 9, lambda **kwargs: '') + ) + except BaseException as error: + errors.append(error) + finally: + sys.settrace(None) + + thread: threading.Thread = threading.Thread(target=render) + thread.start() + try: + assert reached.wait(3) + if mutation == 'assign': + multibar['task'] = replacement + elif mutation == 'update': + multibar.update(task=replacement) + else: + multibar |= {'task': replacement} + finally: + resume.set() + thread.join(3) + assert not thread.is_alive() + assert errors == [] + assert multibar.get('task') is replacement + assert old not in multibar._finished_at + assert old not in multibar._labeled + assert replacement.label == 'task' + + +@pytest.mark.parametrize('removal', ['pop', 'popitem', 'clear', 'del']) +def test_membership_removal_cleans_tracking(removal: str) -> None: + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + progress: progressbar.ProgressBar = multibar['task'] + multibar._finished_at[progress] = 0 + multibar._labeled.add(progress) + if removal == 'pop': + assert multibar.pop('task') is progress + elif removal == 'popitem': + assert multibar.popitem() == ('task', progress) + elif removal == 'clear': + multibar.clear() + else: + del multibar['task'] + assert not multibar + assert not multibar._finished_at + assert not multibar._labeled + + +def test_membership_dict_defaults_and_update_iterable() -> None: + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + progress: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO() + ) + assert multibar.setdefault('task', progress) is progress + assert multibar.setdefault('task') is progress + assert multibar.setdefault('none') is None + assert multibar.pop('none') is None + assert multibar.pop('missing', 42) == 42 + with pytest.raises(KeyError): + multibar.pop('missing') + with pytest.raises(TypeError): + multibar.pop('missing', 1, 2) + multibar.clear() + with pytest.raises(KeyError): + multibar.popitem() + multibar.update([('task', progress)]) + assert multibar['task'] is progress + + +def test_membership_guard_is_released_before_finished_update() -> None: + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + progress: progressbar.ProgressBar = multibar['task'] + unlocked: list[bool] = [] + + def update(**kwargs: object) -> str: + def observer() -> None: + acquired: bool = multibar._membership_lock.acquire(timeout=1) + unlocked.append(acquired) + if acquired: + multibar._membership_lock.release() + + thread: threading.Thread = threading.Thread(target=observer) + thread.start() + thread.join(2) + assert not thread.is_alive() + return '' + + list(multibar._render_finished_bar(progress, 10, None, update)) + assert unlocked == [True] + + +@pytest.mark.parametrize( + 'operation', ['replace', 'pop', 'popitem', 'clear', 'finish'] +) +def test_membership_tracking_never_calls_bar_hash(operation: str) -> None: + class HashBar(progressbar.ProgressBar): + armed: bool = False + + def __hash__(self) -> int: + if self.armed: + raise AssertionError('bar hash callback') + return object.__hash__(self) + + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + progress: HashBar = HashBar(fd=io.StringIO()) + multibar['task'] = progress + multibar._finished_at[progress] = 0 + multibar._labeled.add(progress) + progress.armed = True + if operation == 'replace': + multibar['task'] = progressbar.ProgressBar(fd=io.StringIO()) + elif operation == 'pop': + multibar.pop('task') + elif operation == 'popitem': + multibar.popitem() + elif operation == 'clear': + multibar.clear() + else: + list( + multibar._render_finished_bar( + progress, 10, None, lambda **kwargs: '' + ) + ) + progress.armed = False + + +def test_multibar_membership_lock_resets_after_fork() -> None: + import os + import subprocess + import sys + import textwrap + + if not hasattr(os, 'fork'): + pytest.skip('fork requires POSIX') + code: str = textwrap.dedent(""" + import io + import os + import signal + import threading + import progressbar + + multibar = progressbar.MultiBar(fd=io.StringIO()) + held = threading.Event() + release = threading.Event() + def holder(): + with multibar._membership_lock: + held.set() + release.wait(5) + thread = threading.Thread(target=holder, daemon=True) + thread.start() + assert held.wait(2) + pid = os.fork() + if pid == 0: + signal.alarm(2) + multibar['child'] = progressbar.ProgressBar(fd=io.StringIO()) + os._exit(0) + _, status = os.waitpid(pid, 0) + release.set() + thread.join(2) + assert os.waitstatus_to_exitcode(status) == 0 + """) + result: subprocess.CompletedProcess[str] = subprocess.run( + [sys.executable, '-c', code], + capture_output=True, + text=True, + timeout=15, + ) + assert result.returncode == 0, result.stderr + + +def test_tracking_distinguishes_equal_bars_by_identity() -> None: + class EqualBar(progressbar.ProgressBar): + def __hash__(self) -> int: + raise AssertionError('tracking called hash') + + def __eq__(self, other: object) -> bool: + raise AssertionError('tracking called equality') + + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + first: EqualBar = EqualBar(fd=io.StringIO()) + second: EqualBar = EqualBar(fd=io.StringIO()) + multibar['first'] = first + multibar['second'] = second + multibar._finished_at[first] = 1 + multibar._finished_at[second] = 2 + multibar._labeled.add(first) + multibar._labeled.add(second) + assert len(multibar._finished_at) == len(multibar._labeled) == 2 + assert multibar._finished_at[first] == 1 + assert multibar._finished_at[second] == 2 + assert next(iter(multibar._finished_at)) is first + assert list(multibar._labeled)[1] is second + del multibar['first'] + assert first not in multibar._finished_at + assert first not in multibar._labeled + assert second in multibar._finished_at + assert second in multibar._labeled + multibar.clear() + assert not multibar._finished_at + assert not multibar._labeled + + +class _IdentityTracked: + def __init__(self) -> None: + self.owner: object = None + + def __hash__(self) -> int: + raise AssertionError('hash callback') + + def __eq__(self, other: object) -> bool: + raise AssertionError('equality callback') + + +@pytest.mark.parametrize('method', ['deepcopy', 'pickle']) +@pytest.mark.parametrize('kind', ['mapping', 'set']) +def test_identity_tracking_copy_rebuilds_ids_and_cycles( + method: str, kind: str +) -> None: + import copy + import pickle + import typing + + from progressbar import _identity + + tracked: _IdentityTracked = _IdentityTracked() + collection: typing.Any + if kind == 'mapping': + collection = _identity.IdentityMap() + collection[tracked] = 'value' + else: + collection = _identity.IdentitySet() + collection.add(tracked) + tracked.owner = collection + restored: typing.Any = ( + copy.deepcopy(collection) + if method == 'deepcopy' + else pickle.loads(pickle.dumps(collection)) + ) + copied: _IdentityTracked = next(iter(restored)) + assert copied is not tracked + assert copied.owner is restored + assert copied in restored + assert tracked not in restored + if kind == 'mapping': + assert restored[copied] == 'value' + del restored[copied] + else: + restored.discard(copied) + assert not restored + + +@pytest.mark.parametrize('operation', ['replace', 'clear', 'pop', 'popitem']) +def test_removed_bar_finalizer_runs_outside_membership_guard( + operation: str, +) -> None: + observed: list[bool] = [] + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + + class FinalBar(progressbar.ProgressBar): + def __del__(self) -> None: + def observer() -> None: + acquired: bool = multibar._membership_lock.acquire(timeout=1) + observed.append(acquired) + if acquired: + multibar._membership_lock.release() + + thread: threading.Thread = threading.Thread(target=observer) + thread.start() + thread.join(2) + super().__del__() + + multibar['task'] = FinalBar(fd=io.StringIO()) + multibar._finished_at[multibar['task']] = 0 + multibar._labeled.add(multibar['task']) + if operation == 'replace': + multibar['task'] = progressbar.ProgressBar(fd=io.StringIO()) + elif operation == 'clear': + multibar.clear() + elif operation == 'pop': + multibar.pop('task') + else: + multibar.popitem() + assert observed == [True] + + +def test_render_does_not_relabel_replaced_bar() -> None: + import inspect + import sys + import types + import typing + + from progressbar import multi + + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + old: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO(), widgets=['old'], max_value=1 + ) + replacement: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO() + ) + multibar['task'] = old + old.start() + old.finish() + lines, first = inspect.getsourcelines(multi.MultiBar._render_bar) + pause_line: int = first + next( + i for i, line in enumerate(lines) if 'self._label_bar(bar_' in line + ) + reached: threading.Event = threading.Event() + resume: threading.Event = threading.Event() + errors: list[BaseException] = [] + + def trace( + frame: types.FrameType, event: str, arg: typing.Any + ) -> typing.Any: + if ( + event == 'line' + and frame.f_code.co_filename == multi.__file__ + and frame.f_lineno == pause_line + ): + reached.set() + assert resume.wait(2) + return trace + + def render() -> None: + sys.settrace(trace) + try: + list(multibar._render_bar(old, now=10, expired=None)) + except BaseException as error: + errors.append(error) + finally: + sys.settrace(None) + + thread: threading.Thread = threading.Thread(target=render) + thread.start() + try: + assert reached.wait(2) + multibar['task'] = replacement + finally: + resume.set() + thread.join(2) + assert not thread.is_alive() + assert errors == [] + assert multibar.get('task') is replacement + assert old not in multibar._finished_at + assert old not in multibar._labeled + + +def test_expiry_rechecks_membership_after_custom_interval_callback() -> None: + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + old: progressbar.ProgressBar = multibar['task'] + replacement: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO() + ) + multibar._finished_at[old] = 0 + + class Interval(float): + def __bool__(self) -> bool: + multibar['task'] = replacement + return True + + multibar.remove_finished = Interval(1) + assert ( + list(multibar._render_finished_bar(old, 10, 9, lambda **kwargs: '')) + == [] + ) + assert multibar.get('task') is replacement + + +def test_label_reservation_skips_removed_bars_and_disabled_labels() -> None: + multibar: progressbar.MultiBar = progressbar.MultiBar( + fd=io.StringIO(), prepend_label=False, append_label=False + ) + progress: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO(), widgets=['value'] + ) + multibar['task'] = progress + assert multibar._label_bar(progress, require_member=True) + assert progress.widgets == ['value'] + assert progress not in multibar._labeled + del multibar['task'] + assert not multibar._label_bar(progress, require_member=True) + assert progress not in multibar._labeled diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py index d4859094..8e1393e2 100644 --- a/tests/test_stream_security.py +++ b/tests/test_stream_security.py @@ -1,5 +1,6 @@ from __future__ import annotations +import inspect import io import logging import subprocess @@ -524,27 +525,22 @@ def test_overlapping_stop_capturing_delivers_buffered_text_once( paused: threading.Event = threading.Event() resume: threading.Event = threading.Event() - class PausingBuffer(io.StringIO): - # The first stop pauses between reading and clearing the - # buffer; a second lifecycle must not drain the same text again - def getvalue(self) -> str: - value: str = super().getvalue() - if threading.current_thread().name == 'first-stop': - paused.set() - assert resume.wait(10) - return value - streams: utils.StreamWrapper = utils.StreamWrapper() target: io.StringIO = io.StringIO() streams.stdout = streams.original_stdout = target monkeypatch.setattr(sys, 'stdout', target) wrapper: utils.WrappingIO = streams.wrap_stdout() streams.start_capturing() - wrapper.buffer = PausingBuffer() wrapper.write('spam') - first: threading.Thread = threading.Thread( - target=streams.stop_capturing, name='first-stop' - ) + + def stop() -> None: + sys.settrace(_pause_native_drain(paused, resume)) + try: + streams.stop_capturing() + finally: + sys.settrace(None) + + first: threading.Thread = threading.Thread(target=stop, name='first-stop') first.start() assert paused.wait(10) second_done: threading.Event = threading.Event() @@ -698,6 +694,7 @@ def write(self, text: str) -> int: wrapper: utils.WrappingIO = streams.wrap_stdout() streams.start_capturing() wrapper.write('spam|') + first: threading.Thread = threading.Thread( target=streams.stop_capturing, name='first-stop' ) @@ -763,42 +760,52 @@ def write(self, text: str) -> int: streams.unwrap_stderr() -class _PausingStringIO(io.StringIO): - """A buffer whose first `truncate()` pauses until told to go on.""" +def _pause_native_drain( + paused: threading.Event, resume: threading.Event +) -> typing.Callable[..., typing.Any]: + """Pause real StringIO draining without replacing its methods.""" + lines, first = inspect.getsourcelines(utils.WrappingIO._take) + pause_line: int = first + next( + i for i, line in enumerate(lines) if 'self.buffer.truncate(0)' in line + ) - def __init__(self) -> None: - super().__init__() - self.paused: threading.Event = threading.Event() - self.resume: threading.Event = threading.Event() - self.calls: int = 0 + def trace(frame: typing.Any, event: str, arg: typing.Any) -> typing.Any: + if ( + event == 'line' + and frame.f_code is utils.WrappingIO._take.__code__ + and frame.f_lineno == pause_line + and not paused.is_set() + ): + paused.set() + assert resume.wait(10) + return trace - def truncate(self, size: int | None = None) -> int: - self.calls += 1 - if self.calls == 1: - self.paused.set() - assert self.resume.wait(10) - return super().truncate(size) + return trace def test_write_during_a_drain_is_not_lost() -> None: # Codex-4b finding 7: `_take()` read the buffer, then truncated it, # and a `write()` that landed between the two was gone. wrapper = utils.WrappingIO(io.StringIO(), capturing=True) - buffer = _PausingStringIO() - wrapper.buffer = buffer + paused: threading.Event = threading.Event() + resume: threading.Event = threading.Event() wrapper.write('A|') taken: list[str] = [] def take() -> None: - taken.append(wrapper._take()) + sys.settrace(_pause_native_drain(paused, resume)) + try: + taken.append(wrapper._take()) + finally: + sys.settrace(None) taker = threading.Thread(target=take) taker.start() - assert buffer.paused.wait(10) + assert paused.wait(10) writer = threading.Thread(target=wrapper.write, args=('B|',)) writer.start() writer.join(0.5) # blocked behind the drain, or done without a lock - buffer.resume.set() + resume.set() taker.join(10) writer.join(10) assert not taker.is_alive() and not writer.is_alive() @@ -1022,7 +1029,7 @@ def update_capturing(self) -> None: 'failure_at', ['write', 'target_flush', 'buffer_flush', 'drain'] ) @pytest.mark.parametrize('hook_raises', [False, True]) -def test_failed_stop_still_notifies_hook_and_preserves_first_error( +def test_failed_stop_still_notifies_hook_and_preserves_first_error( # noqa: C901 monkeypatch: pytest.MonkeyPatch, failure_at: str, hook_raises: bool, @@ -1163,3 +1170,170 @@ def check_lock() -> None: assert streams.capturing == 1 assert wrapper.capturing is True streams.unwrap_stdout() + + +@pytest.mark.parametrize('failed', ['stdout', 'stderr']) +@pytest.mark.parametrize( + 'operation', ['flush', 'stop_capturing', 'excepthook'] +) +def test_drain_failure_preserves_other_stream( + failed: str, operation: str +) -> None: + streams: utils.StreamWrapper = utils.StreamWrapper() + targets: dict[str, io.StringIO] = {} + for name in ('stdout', 'stderr'): + targets[name] = io.StringIO() + wrapper: utils.WrappingIO = utils.WrappingIO( + targets[name], capturing=True + ) + setattr(streams, name, wrapper) + setattr(streams, f'wrapped_{name}', 1) + wrapper.write(name) + streams.capturing = 1 + getattr(streams, failed).buffer.close() + streams.original_excepthook = lambda *args: None + with pytest.raises(ValueError, match='closed'): + if operation == 'excepthook': + streams.excepthook(ValueError, ValueError(), None) + else: + getattr(streams, operation)() + healthy: str = 'stderr' if failed == 'stdout' else 'stdout' + assert targets[healthy].getvalue() == healthy + + +@pytest.mark.parametrize('name', ['stdout', 'stderr']) +def test_closed_buffer_unwrap_restores_stream( + name: str, monkeypatch: pytest.MonkeyPatch +) -> None: + streams: utils.StreamWrapper = utils.StreamWrapper() + original: io.StringIO = io.StringIO() + setattr(streams, f'original_{name}', original) + monkeypatch.setattr(sys, name, original) + wrapper: utils.WrappingIO = getattr(streams, f'wrap_{name}')() + wrapper.buffer.close() + try: + getattr(streams, f'unwrap_{name}')() + assert getattr(sys, name) is original + assert getattr(streams, name) is original + assert getattr(streams, f'wrapped_{name}') == 0 + finally: + setattr(streams, name, original) + setattr(streams, f'wrapped_{name}', 0) + streams.unwrap_excepthook() + + +@pytest.mark.parametrize( + 'method', ['write', 'getvalue', 'seek', 'truncate', 'flush'] +) +@pytest.mark.parametrize('instance_shadow', [False, True]) +def test_custom_buffer_methods_run_without_stream_locks( + method: str, instance_shadow: bool +) -> None: + streams: utils.StreamWrapper = utils.StreamWrapper() + target: io.StringIO = io.StringIO() + wrapper: utils.WrappingIO = utils.WrappingIO(target, capturing=True) + streams.stdout = wrapper + streams.wrapped_stdout = 1 + unlocked: list[bool] = [] + native: typing.Callable[..., typing.Any] = getattr(io.StringIO, method) + + def callback(buffer: io.StringIO, *args: typing.Any) -> typing.Any: + def observer() -> None: + for lock in (streams.lock, wrapper._buffer_box.lock): + acquired: bool = lock.acquire(timeout=0.2) + unlocked.append(acquired) + if acquired: + lock.release() + + thread: threading.Thread = threading.Thread(target=observer) + thread.start() + thread.join(2) + assert not thread.is_alive() + return native(buffer, *args) + + if instance_shadow: + wrapper.buffer = io.StringIO() + setattr( + wrapper.buffer, + method, + lambda *args: callback(wrapper.buffer, *args), + ) + else: + buffer_type: type[io.StringIO] = type( + 'CustomBuffer', (io.StringIO,), {method: callback} + ) + wrapper.buffer = buffer_type() + wrapper.write('pending') + streams.flush() + assert unlocked and all(unlocked) + assert target.getvalue() == 'pending' + + +def test_two_drain_failures_preserve_first_exception_and_context() -> None: + failures: list[BaseException] = [ + KeyboardInterrupt('first'), + RuntimeError('second'), + ] + streams: utils.StreamWrapper = utils.StreamWrapper() + for name, failure in zip(('stdout', 'stderr'), failures, strict=True): + wrapper: utils.WrappingIO = utils.WrappingIO(io.StringIO()) + + def fail(error: BaseException = failure) -> str: + raise error + + wrapper.buffer.getvalue = fail # type: ignore[method-assign] + setattr(streams, name, wrapper) + setattr(streams, f'wrapped_{name}', 1) + with pytest.raises(KeyboardInterrupt) as caught: + streams.flush() + assert caught.value is failures[0] + assert caught.value.__context__ is failures[1] + + +@pytest.mark.parametrize('name', ['stdout', 'stderr']) +def test_unwrap_restores_stream_even_when_capture_flag_raises( + name: str, monkeypatch: pytest.MonkeyPatch +) -> None: + failure: KeyboardInterrupt = KeyboardInterrupt('capture flag') + + class Wrapper(utils.WrappingIO): + armed: bool = False + + def __setattr__(self, key: str, value: typing.Any) -> None: + if key == 'capturing' and self.armed: + raise failure + super().__setattr__(key, value) + + streams: utils.StreamWrapper = utils.StreamWrapper() + original: io.StringIO = io.StringIO() + wrapper: Wrapper = Wrapper(original, capturing=True) + wrapper.armed = True + setattr(streams, name, wrapper) + setattr(streams, f'original_{name}', original) + setattr(streams, f'wrapped_{name}', 1) + monkeypatch.setattr(sys, name, wrapper) + with pytest.raises(KeyboardInterrupt) as caught: + getattr(streams, f'unwrap_{name}')() + assert caught.value is failure + assert getattr(sys, name) is original + assert getattr(streams, name) is original + assert getattr(streams, f'wrapped_{name}') == 0 + + +def test_stop_propagation_failure_notifies_hook() -> None: + failure: RuntimeError = RuntimeError('capture propagation') + notifications: list[int] = [] + + class Streams(utils.StreamWrapper): + def _apply_capturing(self) -> list[utils._Pending]: + raise failure + + def update_capturing(self) -> None: + notifications.append(self.capturing) + + streams: Streams = Streams() + streams.capturing = 1 + with pytest.raises(RuntimeError) as caught: + streams.stop_capturing() + assert caught.value is failure + assert notifications == [0] diff --git a/uv.lock b/uv.lock index 2ac3816c..f9a4ed47 100644 --- a/uv.lock +++ b/uv.lock @@ -803,6 +803,7 @@ fast = [ tests = [ { name = "dill" }, { name = "freezegun" }, + { name = "markupsafe" }, { name = "pytest" }, { name = "pytest-cov" }, { name = "pywin32", marker = "sys_platform == 'win32'" }, @@ -818,6 +819,7 @@ requires-dist = [ { name = "dill", marker = "extra == 'tests'", specifier = ">=0.3.6" }, { name = "freezegun", marker = "extra == 'docs-tests'", specifier = ">=0.3.11" }, { name = "freezegun", marker = "extra == 'tests'", specifier = ">=0.3.11" }, + { name = "markupsafe", marker = "extra == 'tests'", specifier = ">=2.0" }, { name = "playwright", marker = "extra == 'docs-tests'", specifier = ">=1.48.0" }, { name = "pytest", marker = "extra == 'docs-tests'", specifier = ">=4.6.9" }, { name = "pytest", marker = "extra == 'tests'", specifier = ">=4.6.9" }, From 2e9c6bf201e6ea8d7c858d2d77f46acee38457d4 Mon Sep 17 00:00:00 2001 From: Rick van Hattem Date: Thu, 29 Oct 2026 11:33:28 +0100 Subject: [PATCH 24/24] Preserve custom label callbacks and chained stream errors --- .pre-commit-config.yaml | 2 + CHANGES.rst | 21 ++-- CONTRIBUTING.rst | 6 +- benchmarks/results.json | 2 +- progressbar/multi.py | 55 +++++++--- progressbar/utils.py | 34 ++++++- tests/test_multibar.py | 184 ++++++++++++++++++++++++++++++++++ tests/test_stream_security.py | 51 ++++++++++ 8 files changed, 329 insertions(+), 26 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 1d38c796..b92dc8aa 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -5,6 +5,8 @@ repos: rev: v6.0.0 hooks: - id: end-of-file-fixer + # Preserve the upstream bytes copied by the vendor update command. + exclude: '^docs/_static/vendor/xterm\.js$' - id: trailing-whitespace - id: check-yaml - id: check-added-large-files diff --git a/CHANGES.rst b/CHANGES.rst index 80b028be..bf81de12 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -15,18 +15,25 @@ unwrapping restores the original stream even when its capture buffer is closed. Built-in ``StringIO`` capture writes and drains remain atomic. Caller-replaced buffers keep their method overrides, which run without stream locks. As in 4.6.0, callers synchronise concurrent writes and drains -on those custom buffers. +on those custom buffers. A capture-hook failure now retains the other +stream's exception in the primary failure's context chain. Specialised format templates retain their escaping and result types. Their field paths, including nested format specifications, are checked before calling the application's formatter. -MultiBar expiry keeps a replacement registered under the same label. -Assignment, update, in-place union and ordinary removal methods share its -membership guard. Lifecycle tracking uses bar identity without calling -application hash or equality methods, and the guard resets after fork. -Direct calls to ``dict`` base-class mutation methods -bypass that guard and are not supported during concurrent rendering. +MultiBar expiry keeps a replacement registered under the same exact +``str`` label. Assignment, update, in-place union and ordinary removal +methods share its membership guard. Lifecycle tracking uses bar identity +without calling application hash or equality methods, and the guard resets +after fork. +Custom label objects retain their hash and equality hooks and their +identity as dictionary keys. Using one for mutation or indexing switches +that MultiBar instance permanently to the 4.6.0 concurrency contract: +callers synchronise concurrent mutation and rendering, including expiry. +These hooks run without the membership guard, even for later exact +``str`` lookups. Clearing the instance does not reset this mode. Direct +calls to ``dict`` base-class mutation methods bypass this protocol. For the most recent changes to the Python Progressbar please look at the Git releases or the commit log: diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 8a900ccf..a8ba6047 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -128,8 +128,10 @@ Ready to contribute? Here's how to set up `python-progressbar` for local develop $ uv run tox Stage the formatted changes before committing. A checkout-local - pre-commit gate, when installed, checks the staged snapshot and leaves - partially staged files untouched. + pre-commit gate, when installed, checks the staged snapshot with current + Ruff and the configured pre-commit hooks, including the CI-pinned Ruff + version. It leaves partially staged files untouched. Configured fixes + run only in the disposable snapshot and cause the commit to be rejected. 6. Commit your changes and push your branch to GitHub with `git-flow-avh`_:: diff --git a/benchmarks/results.json b/benchmarks/results.json index 62515853..83c64411 100644 --- a/benchmarks/results.json +++ b/benchmarks/results.json @@ -114,4 +114,4 @@ } } } -} \ No newline at end of file +} diff --git a/progressbar/multi.py b/progressbar/multi.py index 6259b82d..8bdd53fd 100644 --- a/progressbar/multi.py +++ b/progressbar/multi.py @@ -10,6 +10,7 @@ from __future__ import annotations import collections.abc +import contextlib import enum import importlib import io @@ -37,6 +38,7 @@ importlib.import_module('progressbar.widgets') SortKeyFunc = collections.abc.Callable[[bar.ProgressBar], typing.Any] +_NO_LABEL: object = object() class _Update(typing.Protocol): @@ -81,6 +83,15 @@ class MultiBar(dict[str, bar.ProgressBar]): program can exit. The default preserves the historical wait-forever behavior. + Note: + Exact ``str`` labels retain atomic membership changes and expiry. + Using a custom label for mutation or indexing permanently switches + this instance to the 4.6.0 concurrency contract. Its hash and + equality hooks run without the membership guard, and the caller + synchronises concurrent mutation and rendering. Original key + objects and dictionary equality semantics are retained. Direct + ``dict`` base-method mutation bypasses this protocol. + Note: `fd` is resolved once, from the parameter default, at the time this module is first imported. Unlike a plain `ProgressBar`, @@ -165,6 +176,7 @@ class MultiBar(dict[str, bar.ProgressBar]): _previous_output: list[str] _finished_at: _identity.IdentityMap[bar.ProgressBar, float] _labeled: _identity.IdentitySet[bar.ProgressBar] + _custom_labels: bool _membership_box: utils._SharedLock # pyright: ignore[reportPrivateUsage] _print_lock: threading.RLock _thread: threading.Thread | None @@ -223,6 +235,7 @@ def __init__( self.sort_keyfunc = sort_keyfunc self.sort_reverse = sort_reverse + self._custom_labels = False self._labeled = _identity.IdentitySet[bar.ProgressBar]() self._finished_at = _identity.IdentityMap() self._previous_output = [] @@ -256,6 +269,24 @@ def _membership_lock(self) -> threading.RLock: """Return the membership guard, reset automatically after fork.""" return self._membership_box.lock + @contextlib.contextmanager + def _membership_guard( + self, key: object = _NO_LABEL + ) -> collections.abc.Generator[None, None, None]: + """Guard native labels and hand custom labels back to their caller. + + The switch waits for previous native operations to finish. Every + waiting operation checks the mode after acquisition. Custom mode + stays active after clear because earlier callbacks may still run. + """ + with self._membership_lock: + if key is not _NO_LABEL and type(key) is not str: + self._custom_labels = True + if not self._custom_labels: + yield + return + yield + def __setitem__(self, key: str, bar: bar.ProgressBar) -> None: """Add `bar` to the multibar, rebinding it to draw through us. @@ -286,7 +317,7 @@ def __setitem__(self, key: str, bar: bar.ProgressBar) -> None: bar: The progressbar to add, mutated as described above. """ self._prepare_bar(key, bar) - with self._membership_lock: + with self._membership_guard(key): previous: bar.ProgressBar | None = super().get(key) super().__setitem__(key, bar) if previous is not bar: @@ -309,7 +340,7 @@ def _prepare_bar(self, key: str, progress: bar.ProgressBar) -> None: ) def _forget_bar(self, progress: bar.ProgressBar | None) -> None: - """Discard tracking for a removed member under the guard.""" + """Discard tracking within the current membership contract.""" if progress is not None: self._finished_at.pop(progress, None) self._labeled.discard(progress) @@ -320,7 +351,7 @@ def __delitem__(self, key: str) -> None: def __getitem__(self, key: str) -> bar.ProgressBar: """Get (and create if needed) a progressbar from the multibar.""" - with self._membership_lock: + with self._membership_guard(key): try: return super().__getitem__(key) except KeyError: @@ -353,12 +384,12 @@ def setdefault( self, key: str, default: bar.ProgressBar | None = None ) -> bar.ProgressBar | None: """Insert a missing member without replacing a concurrent one.""" - with self._membership_lock: + with self._membership_guard(key): if super().__contains__(key): return super().__getitem__(key) if default is not None: self._prepare_bar(key, default) - with self._membership_lock: + with self._membership_guard(key): members: dict[str, bar.ProgressBar | None] = typing.cast( dict[str, bar.ProgressBar | None], self ) @@ -368,7 +399,7 @@ def setdefault( def pop(self, key: str, *args: typing.Any) -> typing.Any: """Remove a member and discard its lifecycle tracking.""" - with self._membership_lock: + with self._membership_guard(key): present: bool = super().__contains__(key) progress: typing.Any = super().pop(key, *args) if present: @@ -377,7 +408,7 @@ def pop(self, key: str, *args: typing.Any) -> typing.Any: def popitem(self) -> tuple[str, bar.ProgressBar]: """Remove the newest member and its tracking.""" - with self._membership_lock: + with self._membership_guard(): key: str progress: bar.ProgressBar key, progress = super().popitem() @@ -387,7 +418,7 @@ def popitem(self) -> tuple[str, bar.ProgressBar]: def clear(self) -> None: """Remove all members and their tracking.""" removed: list[bar.ProgressBar] - with self._membership_lock: + with self._membership_guard(): removed = list(super().values()) removed.extend(self._finished_at) removed.extend(self._labeled) @@ -404,7 +435,7 @@ def _label_bar( label: str = bar.label widgets: collections.abc.MutableSequence[typing.Any] = bar.widgets assert widgets, 'Cannot prepend label to empty progressbar' - with self._membership_lock: + with self._membership_guard(label): if require_member and super().get(label) is not bar: return False if bar in self._labeled: # pragma: no branch @@ -548,7 +579,7 @@ def _render_finished_bar( Zero lines (dropped or hidden) or one rendered line. """ label: str = bar_.label - with self._membership_lock: + with self._membership_guard(label): if super().get(label) is not bar_: return finished_at: float | None = self._finished_at.get(bar_) @@ -564,7 +595,7 @@ def _render_finished_bar( and expired is not None and expired >= finished_at ): - with self._membership_lock: + with self._membership_guard(label): if super().get(label) is bar_: super().__delitem__(label) self._forget_bar(bar_) @@ -766,7 +797,7 @@ def get_sorted_bars(self) -> list[bar.ProgressBar]: thread cannot mutate it during the snapshot. Sorting runs after releasing the membership guard. """ - with self._membership_lock: + with self._membership_guard(): bars: list[bar.ProgressBar] = list(super().values()) return sorted(bars, key=self.sort_keyfunc, reverse=self.sort_reverse) diff --git a/progressbar/utils.py b/progressbar/utils.py index 58aad128..18695fca 100644 --- a/progressbar/utils.py +++ b/progressbar/utils.py @@ -14,6 +14,7 @@ import contextlib import datetime import io +import itertools import logging import os import re @@ -511,6 +512,31 @@ def __exit__( self.close() +def _link_exception_context( + primary: BaseException, + secondary: BaseException, + *, + prepend: bool = False, +) -> None: + """Retain distinct context objects once, preserving hook precedence.""" + heads: tuple[BaseException | None, BaseException | None] = ( + (secondary, primary.__context__) + if prepend + else (primary.__context__, secondary) + ) + chain: list[BaseException] = [primary] + seen: set[int] = {id(primary)} + for head in heads: + current: BaseException | None = head + while current is not None and id(current) not in seen: + chain.append(current) + seen.add(id(current)) + current = current.__context__ + for previous, following in itertools.pairwise(chain): + previous.__context__ = following + chain[-1].__context__ = None + + class StreamWrapper: """Wrap `sys.stdout`/`sys.stderr` for output, logging, and a bar to share. @@ -658,9 +684,9 @@ def _notify_capturing(self, failure: BaseException | None) -> None: else: try: self.update_capturing() - finally: - # Keep a hook failure as the original error's context. - raise failure + except BaseException as exception: # noqa: BLE001 + _link_exception_context(failure, exception, prepend=True) + raise failure def update_capturing(self) -> None: # pragma: no cover """Propagate the capturing refcount to the wrapped streams. @@ -736,7 +762,7 @@ def _deliver(self, pending: list[_Pending]) -> None: if failure is None: failure = exception elif exception is not None and exception is not failure: - failure.__context__ = exception + _link_exception_context(failure, exception) if failure is not None: raise failure diff --git a/tests/test_multibar.py b/tests/test_multibar.py index d32fd760..b8ea8217 100644 --- a/tests/test_multibar.py +++ b/tests/test_multibar.py @@ -1018,3 +1018,187 @@ def test_label_reservation_skips_removed_bars_and_disabled_labels() -> None: del multibar['task'] assert not multibar._label_bar(progress, require_member=True) assert progress not in multibar._labeled + + +def _observe_membership_lock( + multibar: progressbar.MultiBar, observed: list[bool] +) -> None: + def acquire() -> None: + acquired: bool = multibar._membership_lock.acquire(timeout=0.1) + observed.append(acquired) + if acquired: + multibar._membership_lock.release() + + thread: threading.Thread = threading.Thread(target=acquire) + thread.start() + thread.join(2) + assert not thread.is_alive() + + +@pytest.mark.parametrize( + 'operation', ['assign', 'lookup', 'pop', 'setdefault', 'label', 'expire'] +) +@pytest.mark.parametrize('callback', ['hash', 'equal']) +def test_custom_label_callbacks_run_outside_membership_guard( + operation: str, callback: str +) -> None: + multibar: progressbar.MultiBar = progressbar.MultiBar( + fd=io.StringIO(), remove_finished=1 + ) + observed: list[bool] = [] + armed: bool = False + + class Label(str): + def __hash__(self) -> int: + if armed and callback == 'hash': + _observe_membership_lock(multibar, observed) + return str.__hash__(self) + + def __eq__(self, other: object) -> bool: + if armed and callback == 'equal': + _observe_membership_lock(multibar, observed) + return str.__eq__(self, other) + + label: Label = Label('task') + progress: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO(), widgets=['x'] + ) + multibar[label] = progress + assert next(iter(multibar)) is label + # Exact-str queries still call equality on the retained custom key. + key: str = label if callback == 'hash' else 'task' + armed = True + if operation == 'assign': + multibar[key] = progressbar.ProgressBar(fd=io.StringIO()) + assert next(iter(multibar)) is label + elif operation == 'lookup': + assert multibar[key] is progress + elif operation == 'pop': + assert multibar.pop(key) is progress + elif operation == 'setdefault': + assert multibar.setdefault(key) is progress + elif operation == 'label': + progress.label = key + multibar._label_bar(progress, require_member=True) + else: + progress.label = key + multibar._finished_at[progress] = 0 + list( + multibar._render_finished_bar(progress, 10, 9, lambda **kwargs: '') + ) + assert observed and all(observed) + + +def test_waiting_native_lookup_rechecks_custom_mode() -> None: + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + observed: list[bool] = [] + started: threading.Event = threading.Event() + errors: list[BaseException] = [] + armed: bool = False + + class Label(str): + __hash__ = str.__hash__ + + def __eq__(self, other: object) -> bool: + if armed: + + def acquire() -> None: + acquired: bool = multibar._membership_lock.acquire( + timeout=0.2 + ) + observed.append(acquired) + if acquired: + multibar._membership_lock.release() + + observer: threading.Thread = threading.Thread(target=acquire) + observer.start() + observer.join(2) + return str.__eq__(self, other) + + progress: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO() + ) + + def lookup() -> None: + started.set() + try: + assert multibar['task'] is progress + except BaseException as error: + errors.append(error) + + with multibar._membership_lock: + thread: threading.Thread = threading.Thread(target=lookup) + thread.start() + assert started.wait(2) + multibar[Label('task')] = progress + armed = True + thread.join(2) + assert not thread.is_alive() + assert errors == [] + assert observed and all(observed) + + +def test_clear_does_not_reenable_native_mode_during_custom_insert() -> None: + multibar: progressbar.MultiBar = progressbar.MultiBar(fd=io.StringIO()) + hashing: threading.Event = threading.Event() + resume: threading.Event = threading.Event() + observed: list[bool] = [] + errors: list[BaseException] = [] + armed: bool = False + + class Label(str): + def __hash__(self) -> int: + if ( + threading.current_thread().name == 'custom-insert' + and not hashing.is_set() + ): + hashing.set() + assert resume.wait(3) + return str.__hash__(self) + + def __eq__(self, other: object) -> bool: + if armed: + + def acquire() -> None: + acquired: bool = multibar._membership_lock.acquire( + timeout=0.2 + ) + observed.append(acquired) + if acquired: + multibar._membership_lock.release() + + observer: threading.Thread = threading.Thread(target=acquire) + observer.start() + observer.join(2) + return str.__eq__(self, other) + + label: Label = Label('task') + progress: progressbar.ProgressBar = progressbar.ProgressBar( + fd=io.StringIO() + ) + + def insert() -> None: + try: + multibar[label] = progress + except BaseException as error: + errors.append(error) + + thread: threading.Thread = threading.Thread( + target=insert, name='custom-insert' + ) + thread.start() + try: + assert hashing.wait(2) + # On the old code this lock remains held by the custom hash callback. + acquired: bool = multibar._membership_lock.acquire(timeout=0.2) + assert acquired + multibar._membership_lock.release() + multibar.clear() + finally: + resume.set() + thread.join(3) + assert errors == [] + armed = True + assert multibar['task'] is progress + assert next(iter(multibar)) is label + assert observed and all(observed) diff --git a/tests/test_stream_security.py b/tests/test_stream_security.py index 8e1393e2..e5be65d3 100644 --- a/tests/test_stream_security.py +++ b/tests/test_stream_security.py @@ -1337,3 +1337,54 @@ def update_capturing(self) -> None: streams.stop_capturing() assert caught.value is failure assert notifications == [0] + + +@pytest.mark.parametrize('hook_error', ['distinct', 'primary', 'secondary']) +@pytest.mark.parametrize('with_context', [False, True]) +def test_capture_hook_retains_both_drain_failure_contexts( + hook_error: str, with_context: bool +) -> None: + first: KeyboardInterrupt = KeyboardInterrupt('stdout drain') + second: RuntimeError = RuntimeError('stderr drain') + third: BaseException = { + 'distinct': OSError('capture hook'), + 'primary': first, + 'secondary': second, + }[hook_error] + origins: list[BaseException] = [] + if with_context: + origins = [LookupError('first origin'), LookupError('second origin')] + first.__context__, second.__context__ = origins + if hook_error == 'distinct': + origin: LookupError = LookupError('hook origin') + third.__context__ = origin + origins.append(origin) + + class Streams(utils.StreamWrapper): + def update_capturing(self) -> None: + raise third + + streams: Streams = Streams() + for name, error in (('stdout', first), ('stderr', second)): + wrapper: utils.WrappingIO = utils.WrappingIO(io.StringIO()) + + def fail(exception: BaseException = error) -> str: + raise exception + + wrapper.buffer.getvalue = fail # type: ignore[method-assign] + setattr(streams, name, wrapper) + setattr(streams, f'wrapped_{name}', 1) + with pytest.raises(KeyboardInterrupt) as caught: + streams.stop_capturing() + assert caught.value is first + chain: list[BaseException] = [] + current: BaseException | None = caught.value + while current is not None: + assert all(current is not previous for previous in chain) + chain.append(current) + current = current.__context__ + assert second in chain + assert third in chain + assert all(origin in chain for origin in origins) + if hook_error == 'distinct': + assert first.__context__ is third