From 0294bff7276b50191914888a3814b0d850ebec94 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Fri, 28 Oct 2022 11:37:33 -0400 Subject: [PATCH 1/3] Write test to demonstrate job hang on pipes --- test/unit/utils/test_utils.py | 45 +++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/unit/utils/test_utils.py b/test/unit/utils/test_utils.py index b83441e6c..5e165e7ed 100644 --- a/test/unit/utils/test_utils.py +++ b/test/unit/utils/test_utils.py @@ -4,6 +4,7 @@ import os import signal import time +import stat from pathlib import Path @@ -113,6 +114,50 @@ def test_transmit_symlink(tmp_path, symlink_dest, check_content): assert f.read() == 'hello world' +@pytest.mark.timeout(timeout=3) +def test_stream_dir_no_hang_on_pipe(tmp_path): + # prepare the input private_data_dir directory to zip + pdd = tmp_path / 'timeout_test' + pdd.mkdir() + + with open(pdd / 'ordinary_file.txt', 'w') as f: + f.write('hello world') + + # make pipe, similar to open_fifo_write + os.mkfifo(pdd / 'my_pipe', stat.S_IRUSR | stat.S_IWUSR) + + # zip and stream the data into the in-memory buffer outgoing_buffer + outgoing_buffer = io.BytesIO() + outgoing_buffer.name = 'not_stdout' + stream_dir(pdd, outgoing_buffer) + + +@pytest.mark.timeout(timeout=3) +def test_unstream_dir_no_hang_on_pipe(tmp_path): + # prepare the input private_data_dir directory to zip + pdd = tmp_path / 'timeout_test_source_dir' + pdd.mkdir() + + with open(pdd / 'ordinary_file.txt', 'w') as f: + f.write('hello world') + + # zip and stream the data into the in-memory buffer outgoing_buffer + outgoing_buffer = io.BytesIO() + outgoing_buffer.name = 'not_stdout' + stream_dir(pdd, outgoing_buffer) + + dest_dir = tmp_path / 'timeout_test_dest' + dest_dir.mkdir() + + # We create the pipe in the same location as an archived file to trigger the bug + os.mkfifo(dest_dir / 'ordinary_file.txt', stat.S_IRUSR | stat.S_IWUSR) + + outgoing_buffer.seek(0) + first_line = outgoing_buffer.readline() + size_data = json.loads(first_line.strip()) + unstream_dir(outgoing_buffer, size_data['zipfile'], dest_dir) + + @pytest.mark.parametrize('fperm', [ 0o777, 0o666, From d496b41f35f525ece106ac9f4f0785973605c524 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Fri, 16 Sep 2022 17:24:45 -0400 Subject: [PATCH 2/3] Remove any pipe files before zip/unzipping - Prevents hangs when zipping, unzipping artifacts dir. If python attempts to open a fifo pipe, it will block indefinitely until it can read data from the pipe. --- ansible_runner/utils/streaming.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ansible_runner/utils/streaming.py b/ansible_runner/utils/streaming.py index 7715e9576..f75bda499 100644 --- a/ansible_runner/utils/streaming.py +++ b/ansible_runner/utils/streaming.py @@ -22,6 +22,11 @@ def stream_dir(source_directory, stream): relpath = "" for fname in files + dirs: full_path = os.path.join(dirpath, fname) + if stat.S_ISFIFO(os.stat(full_path).st_mode): + # skip any pipes, as python hangs when attempting + # to read them. + # i.e. ssh_key_data that was never cleaned up + continue # Magic to preserve symlinks if os.path.islink(full_path): archive_relative_path = os.path.relpath(dirpath, source_directory) @@ -79,6 +84,11 @@ def unstream_dir(stream, length, target_directory): is_symlink = mode[:1] == 'l' if os.path.exists(out_path): + if stat.S_ISFIFO(os.stat(out_path).st_mode): + # must remove any pipes before reading + # i.e. ssh_key_data that was never cleaned up + os.remove(out_path) + continue if is_symlink: os.remove(out_path) elif os.path.isdir(out_path): From df3582dd53b89cd0c78aee6ae4a35663876ee986 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Mon, 24 Oct 2022 16:30:58 -0400 Subject: [PATCH 3/3] move fifo check below symlink --- ansible_runner/utils/streaming.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/ansible_runner/utils/streaming.py b/ansible_runner/utils/streaming.py index f75bda499..92a708fbc 100644 --- a/ansible_runner/utils/streaming.py +++ b/ansible_runner/utils/streaming.py @@ -22,11 +22,6 @@ def stream_dir(source_directory, stream): relpath = "" for fname in files + dirs: full_path = os.path.join(dirpath, fname) - if stat.S_ISFIFO(os.stat(full_path).st_mode): - # skip any pipes, as python hangs when attempting - # to read them. - # i.e. ssh_key_data that was never cleaned up - continue # Magic to preserve symlinks if os.path.islink(full_path): archive_relative_path = os.path.relpath(dirpath, source_directory) @@ -37,6 +32,11 @@ def stream_dir(source_directory, stream): permissions |= 0xA000 zip_info.external_attr = permissions << 16 archive.writestr(zip_info, os.readlink(full_path)) + elif stat.S_ISFIFO(os.stat(full_path).st_mode): + # skip any pipes, as python hangs when attempting + # to open them. + # i.e. ssh_key_data that was never cleaned up + continue else: archive.write( os.path.join(dirpath, fname), arcname=os.path.join(relpath, fname) @@ -84,13 +84,14 @@ def unstream_dir(stream, length, target_directory): is_symlink = mode[:1] == 'l' if os.path.exists(out_path): - if stat.S_ISFIFO(os.stat(out_path).st_mode): - # must remove any pipes before reading + if is_symlink: + os.remove(out_path) + elif stat.S_ISFIFO(os.stat(out_path).st_mode): + # remove any pipes, as python hangs when attempting + # to open them. # i.e. ssh_key_data that was never cleaned up os.remove(out_path) continue - if is_symlink: - os.remove(out_path) elif os.path.isdir(out_path): # Special case, the important dirs were pre-created so don't try to chmod them continue