aws / aws/aws-cli

aws history show/list: UnboundLocalError instead of clear error when pager fails to start

Open Beginner friendly
#10,581 0 comments 0 reactions 0 assignees View on GitHub
needs-triage
Dominant language
Python
Stars
17.3k
Forks
4.6k
Avg merge
1d 2h
Merged PRs (30d)
13

Description

## Describe the bug

`OutputStreamFactory.get_pager_stream` (`awscli/utils.py`) is used by `aws history show` / `aws history list` to pipe output through a pager when running in a TTY. The `try`/`except OSError`/`finally` structure is meant to gracefully ignore `OSError`s (e.g. broken pipe when the pager is closed abruptly):

```python
@contextlib.contextmanager
def get_pager_stream(self, preferred_pager=None):
popen_kwargs = self._get_process_pager_kwargs(preferred_pager)
try:
process = self._popen(**popen_kwargs)
yield process.stdin
except OSError:
# Ignore IOError since this can commonly be raised when a pager
# is closed abruptly and causes a broken pipe.
pass
finally:
process.communicate()
```

The `try` block covers both process creation (`self._popen(...)`) and process usage (`yield process.stdin`). If `self._popen(...)` itself raises `OSError` — which happens whenever the configured pager binary doesn't exist (`FileNotFoundError` is an `OSError` subclass), e.g. `AWS_PAGER`/`PAGER` is misconfigured, or a minimal system has no `less` installed — then `process` is never assigned. The `except OSError: pass` swallows that exception, but the `finally: process.communicate()` then references the undefined local variable `process`, raising an unrelated `UnboundLocalError` instead of a clear error about the pager.

## Repro

```python
from awscli.utils import OutputStreamFactory

def bad_popen(*args, **kwargs):
raise OSError('no such file or directory: nonexistentpager')

factory = OutputStreamFactory(bad_popen)
with factory.get_pager_stream('nonexistentpager'):
pass
```

Output:
```
Traceback (most recent call last):
...
File "awscli/utils.py", line 253, in get_pager_stream
process.communicate()
UnboundLocalError: cannot access local variable 'process' where it is not associated with a value
```

## Real-world trigger

Any user of `aws history show`/`aws history list` running in a TTY with `AWS_PAGER`/`PAGER` set to a nonexistent or misspelled command gets this confusing `UnboundLocalError` traceback instead of a clear "pager not found" style error.

## Suggested fix

Only wrap the `yield` (process usage) in the try/except that's meant to swallow broken-pipe errors; let a failure to start the process propagate normally:

```python
process = self._popen(**popen_kwargs)
try:
yield process.stdin
except OSError:
pass
finally:
process.communicate()
```

I have a PR ready with this fix plus a regression test in `tests/unit/test_utils.py::TestOutputStreamFactory` that reproduces the `UnboundLocalError` against the current code and passes with the fix.

## Environment

- `aws-cli` develop branch (current)
- Confirmed no existing test in `tests/unit/test_utils.py::TestOutputStreamFactory` exercises `self._popen(...)` itself raising `OSError` (existing `test_can_silence_io_error_from_pager` only covers `process.stdin` access raising after `Popen` succeeds).

Contributor guide

Open the contributing guide

Research direction

Start in awscli/utils.py at OutputStreamFactory.get_pager_stream, then read tests/unit/test_utils.py::TestOutputStreamFactory and its existing pager error test. Run the focused tests and confirm a pager-start failure reports the original error rather than UnboundLocalError while the broken-pipe case remains covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
aws, python
Domain
cli
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.