From b4d7279b02ff84b745007c42cbbc940e69b735b9 Mon Sep 17 00:00:00 2001 From: HenrikGharagyozyan1 Date: Fri, 7 Aug 2026 17:48:40 +0400 Subject: [PATCH] Handle broken pipe when output is piped to early-exiting reader Piping a command's output into a reader that exits early, such as "aws s3 ls s3://bucket/ | head -1", printed "[Errno 32] Broken pipe" along with an "Exception ignored" traceback and exited non-zero. BrokenPipeError from writing to the closed pipe reached the blanket except clause in CLIDriver.main() and was handled by GeneralExceptionHandler. The interpreter's final stdout flush then failed on the same pipe, which emitted the traceback and replaced the exit status with Python's own flush failure code. Add a BrokenPipeExceptionHandler to both handler chains, modelled on the existing InterruptExceptionHandler, returning 128 + SIGPIPE to match what standard Unix utilities report for a closed pipe. Point the stdout descriptor at devnull before returning so the shutdown flush cannot fail. Restoring signal.SIG_DFL for SIGPIPE was considered and rejected: Python sets SIGPIPE to SIG_IGN at startup so socket writes raise EPIPE as catchable exceptions, and restoring the default would make broken socket writes terminate the process instead of being retried. --- .changes/next-release/bugfix-s3-62818.json | 5 ++ awscli/errorhandler.py | 43 +++++++++++ awscli/topics/return-codes.rst | 5 ++ tests/functional/s3/test_ls_command.py | 38 +++++++++ tests/unit/test_errorhandler.py | 90 ++++++++++++++++++++++ 5 files changed, 181 insertions(+) create mode 100644 .changes/next-release/bugfix-s3-62818.json diff --git a/.changes/next-release/bugfix-s3-62818.json b/.changes/next-release/bugfix-s3-62818.json new file mode 100644 index 000000000000..107b58c2b9e0 --- /dev/null +++ b/.changes/next-release/bugfix-s3-62818.json @@ -0,0 +1,5 @@ +{ + "type": "bugfix", + "category": "s3", + "description": "Fix ``aws s3 ls`` reporting an unhandled ``Broken pipe`` error when its output is piped to a command that exits early, such as ``head``. fixes `#5899 `__" +} diff --git a/awscli/errorhandler.py b/awscli/errorhandler.py index e09d4c740220..193b0d1b1ba0 100644 --- a/awscli/errorhandler.py +++ b/awscli/errorhandler.py @@ -11,8 +11,11 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import argparse +import io import logging +import os import signal +import sys from botocore.exceptions import ( ClientError, @@ -44,6 +47,10 @@ LOG = logging.getLogger(__name__) VALID_ERROR_FORMATS = ['legacy', 'json', 'yaml', 'text', 'table', 'enhanced'] +# Windows has no SIGPIPE, but writing to a closed pipe can still raise a +# BrokenPipeError there. Falling back to the POSIX signal number keeps the +# return code the same on every platform. +BROKEN_PIPE_RC = 128 + getattr(signal, 'SIGPIPE', 13) # Maximum number of items to display inline for collections MAX_INLINE_ITEMS = 5 @@ -108,6 +115,7 @@ def construct_entry_point_handlers_chain(): ParamValidationErrorsHandler(), PrompterInterruptExceptionHandler(), InterruptExceptionHandler(), + BrokenPipeExceptionHandler(), GeneralExceptionHandler(), ] return ChainedExceptionHandler(exception_handlers=handlers) @@ -124,6 +132,7 @@ def construct_cli_error_handlers_chain(session=None): NoCredentialsErrorHandler(session), PagerErrorHandler(session), InterruptExceptionHandler(), + BrokenPipeExceptionHandler(session), ClientErrorHandler(session), GeneralExceptionHandler(session), ] @@ -388,6 +397,40 @@ def _do_handle_exception(self, exception, stdout, stderr, **kwargs): return self.RC +def _redirect_stdout_to_devnull(): + """Point the stdout file descriptor at devnull. + + Without this, the interpreter's final flush of stdout during shutdown + fails on the closed pipe. That prints an "Exception ignored ..." + traceback and makes Python exit with its own flush failure status + instead of the return code reported by the handler. + """ + try: + fileno = sys.stdout.fileno() + except (AttributeError, ValueError, io.UnsupportedOperation): + # stdout has been replaced by an object with no underlying file + # descriptor, so there is nothing to redirect. + return + devnull = os.open(os.devnull, os.O_WRONLY) + try: + os.dup2(devnull, fileno) + finally: + os.close(devnull) + + +class BrokenPipeExceptionHandler(FilteredExceptionHandler): + EXCEPTIONS_TO_HANDLE = BrokenPipeError + RC = BROKEN_PIPE_RC + + def _do_handle_exception(self, exception, stdout, stderr, **kwargs): + # A reader that closes the pipe early is a normal way for a command + # to end, e.g. "aws s3 ls s3://bucket/ | head -1". Unix utilities + # exit quietly with 128 + SIGPIPE in this situation, so nothing is + # written to stderr here. + _redirect_stdout_to_devnull() + return self.RC + + class GeneralExceptionHandler(FilteredExceptionHandler): EXCEPTIONS_TO_HANDLE = Exception RC = GENERAL_ERROR_RC diff --git a/awscli/topics/return-codes.rst b/awscli/topics/return-codes.rst index 030b7ad5e041..9f93f3f50763 100644 --- a/awscli/topics/return-codes.rst +++ b/awscli/topics/return-codes.rst @@ -31,6 +31,11 @@ of a CLI command: * ``130`` -- The process received a SIGINT (Ctrl-C). +* ``141`` -- The command was writing its output to a pipe that was closed + before the output was fully written, for example when piping to a command + such as ``head`` that exits after reading the lines it needs. This matches + the exit status that standard Unix utilities report for a closed pipe. + * ``252`` -- Command syntax was invalid, an unknown parameter was provided, or a parameter value was incorrect and prevented the command from running. diff --git a/tests/functional/s3/test_ls_command.py b/tests/functional/s3/test_ls_command.py index 0220b40a3f85..9d3faf369aa0 100644 --- a/tests/functional/s3/test_ls_command.py +++ b/tests/functional/s3/test_ls_command.py @@ -11,8 +11,11 @@ # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +import errno + from dateutil import parser, tz +from awscli.testutils import mock from tests.functional.s3 import BaseS3TransferCommandTest @@ -417,3 +420,38 @@ def test_list_objects_ignores_bucket_region(self): ) call_args = self.operations_called[0][1] self.assertNotIn('BucketRegion', call_args) + + +class TestLSCommandBrokenPipe(BaseS3TransferCommandTest): + """Regression tests for aws/aws-cli#5899. + + Piping ``s3 ls`` into a reader that exits early, such as + ``aws s3 ls s3://bucket/ | head -1``, closes the pipe while the listing + is still being written. That must end quietly rather than reporting an + unhandled error. + """ + + def setUp(self): + super().setUp() + self.parsed_responses = [ + { + "CommonPrefixes": [], + "Contents": [ + { + "Key": "foo/bar.txt", + "Size": 100, + "LastModified": "2014-01-09T20:45:49.000Z", + } + ], + } + ] + + def test_closed_pipe_exits_with_sigpipe_rc_and_no_error(self): + with mock.patch( + 'awscli.customizations.s3.subcommands.uni_print', + side_effect=BrokenPipeError(errno.EPIPE, 'Broken pipe'), + ): + _, stderr, _ = self.run_cmd( + 's3 ls s3://bucket/', expected_rc=128 + 13 + ) + self.assertEqual(stderr, '') diff --git a/tests/unit/test_errorhandler.py b/tests/unit/test_errorhandler.py index 37a6e579e76d..a207a3f3a1ea 100644 --- a/tests/unit/test_errorhandler.py +++ b/tests/unit/test_errorhandler.py @@ -10,7 +10,9 @@ # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +import errno import io +import signal from collections import namedtuple import pytest @@ -32,9 +34,12 @@ ConfigurationError, ParamValidationError, ) +from awscli.testutils import mock Case = namedtuple('Case', ['exception', 'rc', 'stderr', 'stdout']) +BROKEN_PIPE_RC = 128 + 13 + def _assert_rc_and_error_message(case, error_handler): stderr = io.StringIO() @@ -127,3 +132,88 @@ def test_cli_error_handling_chain_injection(case): def test_entry_point_error_handling_chain(case): error_handler = errorhandler.construct_entry_point_handlers_chain() _assert_rc_and_error_message(case, error_handler) + + +@pytest.fixture +def broken_pipe_error(): + return BrokenPipeError(errno.EPIPE, 'Broken pipe') + + +@pytest.fixture +def no_stdout_redirect(): + # The handler replaces the process wide stdout file descriptor, which + # would swallow the output of the test runner itself. Tests that care + # about the redirect exercise it directly instead. + with mock.patch.object( + errorhandler, '_redirect_stdout_to_devnull' + ) as patched: + yield patched + + +@pytest.mark.parametrize( + "chain_factory", + [ + errorhandler.construct_entry_point_handlers_chain, + errorhandler.construct_cli_error_handlers_chain, + ], +) +def test_broken_pipe_is_handled_quietly( + chain_factory, broken_pipe_error, no_stdout_redirect +): + # A closed downstream pipe is a normal way for a command to end, so it + # should not produce any error output. See aws/aws-cli#5899. + stdout = io.StringIO() + stderr = io.StringIO() + + rc = chain_factory().handle_exception(broken_pipe_error, stdout, stderr) + + assert rc == BROKEN_PIPE_RC + assert stderr.getvalue() == '' + assert stdout.getvalue() == '' + + +def test_broken_pipe_rc_matches_sigpipe(): + # 128 + SIGPIPE is what standard Unix utilities report for a closed pipe. + assert errorhandler.BROKEN_PIPE_RC == 128 + signal.SIGPIPE + + +def test_broken_pipe_redirects_stdout(broken_pipe_error, no_stdout_redirect): + errorhandler.BrokenPipeExceptionHandler().handle_exception( + broken_pipe_error, io.StringIO(), io.StringIO() + ) + no_stdout_redirect.assert_called_once_with() + + +def test_unrelated_os_error_still_reported(no_stdout_redirect): + # BrokenPipeError is an OSError subclass, so make sure the new handler + # does not start silencing other OSErrors. + stdout = io.StringIO() + stderr = io.StringIO() + error = OSError(errno.EACCES, 'Permission denied') + + rc = errorhandler.construct_entry_point_handlers_chain().handle_exception( + error, stdout, stderr + ) + + assert rc == 255 + assert 'Permission denied' in stderr.getvalue() + no_stdout_redirect.assert_not_called() + + +def test_redirect_stdout_to_devnull_discards_writes(tmp_path): + # Use a real file as a stand in for stdout so that the file descriptor + # belonging to the test runner is never touched. + path = tmp_path / 'stdout.txt' + with open(path, 'w') as fake_stdout: + with mock.patch('sys.stdout', fake_stdout): + errorhandler._redirect_stdout_to_devnull() + fake_stdout.write('discarded') + + assert path.read_text() == '' + + +def test_redirect_stdout_to_devnull_without_fileno(): + # capture_output() and friends replace stdout with an in memory stream + # that has no file descriptor, which must not raise. + with mock.patch('sys.stdout', io.StringIO()): + errorhandler._redirect_stdout_to_devnull()