From e9935cbcf62f7d2f83b9234f6db4e41a3ca480cb Mon Sep 17 00:00:00 2001 From: Jakub Jelen <jjelen@redhat.com> Date: Fri, 30 Aug 2024 12:01:52 +0200 Subject: [PATCH 1/8] sftp: Fix downloading files Fixes: #341 Signed-off-by: Jakub Jelen <jjelen@redhat.com> --- src/pylibsshext/sftp.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pylibsshext/sftp.pyx b/src/pylibsshext/sftp.pyx index 6331c529d..98077a00b 100644 --- a/src/pylibsshext/sftp.pyx +++ b/src/pylibsshext/sftp.pyx @@ -100,7 +100,7 @@ cdef class SFTP: raise LibsshSFTPException("Reading data from remote file [%s] failed with error [%s]" % (remote_file, self._get_sftp_error_str())) - with open(local_file, 'wb+') as f: + with open(local_file, 'ab') as f: bytes_written = f.write(read_buffer[:file_data]) if bytes_written and file_data != bytes_written: sftp.sftp_close(rf) From 9001f9ae0f8dd61c5cca9442d6f8e77becc1be6f Mon Sep 17 00:00:00 2001 From: Jakub Jelen <jjelen@redhat.com> Date: Fri, 30 Aug 2024 11:20:00 +0200 Subject: [PATCH 2/8] tests: Reproducer for #341 Signed-off-by: Jakub Jelen <jjelen@redhat.com> --- tests/unit/sftp_test.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/unit/sftp_test.py b/tests/unit/sftp_test.py index fc70c4512..88add8a56 100644 --- a/tests/unit/sftp_test.py +++ b/tests/unit/sftp_test.py @@ -2,6 +2,8 @@ """Tests suite for sftp.""" +import random +import string import uuid import pytest @@ -63,3 +65,29 @@ def test_get(dst_path, src_path, sftp_session, transmit_payload): """Check that SFTP file download works.""" sftp_session.get(str(src_path), str(dst_path)) assert dst_path.read_bytes() == transmit_payload + + +@pytest.fixture +def large_payload(): + """Generate a large 1025 byte (1024 + 1B) test payload.""" + payload_len = 1024 + 1 + random_bytes = [ord(random.choice(string.printable)) for _ in range(payload_len)] + return bytes(random_bytes) + + +@pytest.fixture +def src_path_large(tmp_path, large_payload): + """Return a remote path to a 1025 byte-sized file. + + The pylibssh chunk size is 1024 so the test needs a file that would + execute at least two loops. + """ + path = tmp_path / 'large.txt' + path.write_bytes(large_payload) + return path + + +def test_get_large(dst_path, src_path_large, sftp_session, large_payload): + """Check that SFTP can download large file.""" + sftp_session.get(str(src_path_large), str(dst_path)) + assert dst_path.read_bytes() == large_payload From 09df17e7d024c74490e6d8c45f13f38b7ecb0137 Mon Sep 17 00:00:00 2001 From: Jakub Jelen <jjelen@redhat.com> Date: Fri, 30 Aug 2024 11:23:57 +0200 Subject: [PATCH 3/8] tests: Symmetric reproducer for upload Signed-off-by: Jakub Jelen <jjelen@redhat.com> --- tests/unit/sftp_test.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/unit/sftp_test.py b/tests/unit/sftp_test.py index 88add8a56..1572c0d66 100644 --- a/tests/unit/sftp_test.py +++ b/tests/unit/sftp_test.py @@ -87,6 +87,12 @@ def src_path_large(tmp_path, large_payload): return path +def test_put_large(dst_path, src_path_large, sftp_session, large_payload): + """Check that SFTP can upload large file.""" + sftp_session.put(str(src_path_large), str(dst_path)) + assert dst_path.read_bytes() == large_payload + + def test_get_large(dst_path, src_path_large, sftp_session, large_payload): """Check that SFTP can download large file.""" sftp_session.get(str(src_path_large), str(dst_path)) From 946abfe81ae72f4d0b087867c18b43b2b8f40e98 Mon Sep 17 00:00:00 2001 From: Jakub Jelen <jjelen@redhat.com> Date: Fri, 15 Nov 2024 14:08:48 +0100 Subject: [PATCH 4/8] sftp: Fix overwriting existing files while downloading Thanks @kucharskim for the report and testing! Signed-off-by: Jakub Jelen <jjelen@redhat.com> --- src/pylibsshext/sftp.pyx | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/pylibsshext/sftp.pyx b/src/pylibsshext/sftp.pyx index 98077a00b..0b42ff8f4 100644 --- a/src/pylibsshext/sftp.pyx +++ b/src/pylibsshext/sftp.pyx @@ -89,18 +89,19 @@ cdef class SFTP: rf = sftp.sftp_open(self._libssh_sftp_session, remote_file_b, O_RDONLY, sftp.S_IRWXU) if rf is NULL: - raise LibsshSFTPException("Opening remote file [%s] for read failed with error [%s]" % (remote_file, self._get_sftp_error_str())) - - while True: - file_data = sftp.sftp_read(rf, <void *>read_buffer, sizeof(char) * 1024) - if file_data == 0: - break - elif file_data < 0: - sftp.sftp_close(rf) - raise LibsshSFTPException("Reading data from remote file [%s] failed with error [%s]" - % (remote_file, self._get_sftp_error_str())) - - with open(local_file, 'ab') as f: + raise LibsshSFTPException("Opening remote file [%s] for read failed with error [%s]" + % (remote_file, self._get_sftp_error_str())) + + with open(local_file, 'wb') as f: + while True: + file_data = sftp.sftp_read(rf, <void *>read_buffer, sizeof(char) * 1024) + if file_data == 0: + break + elif file_data < 0: + sftp.sftp_close(rf) + raise LibsshSFTPException("Reading data from remote file [%s] failed with error [%s]" + % (remote_file, self._get_sftp_error_str())) + bytes_written = f.write(read_buffer[:file_data]) if bytes_written and file_data != bytes_written: sftp.sftp_close(rf) From 945ae7ccfe9f2c473c7b529d44fbc22fb1c3576a Mon Sep 17 00:00:00 2001 From: Jakub Jelen <jjelen@redhat.com> Date: Fri, 15 Nov 2024 14:09:43 +0100 Subject: [PATCH 5/8] tests: Reproduer for overriding existing files Signed-off-by: Jakub Jelen <jjelen@redhat.com> --- tests/unit/sftp_test.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/unit/sftp_test.py b/tests/unit/sftp_test.py index 1572c0d66..93446bd9f 100644 --- a/tests/unit/sftp_test.py +++ b/tests/unit/sftp_test.py @@ -50,6 +50,22 @@ def dst_path(file_paths_pair): return path +@pytest.fixture +def other_payload(): + """Generate a binary test payload.""" + uuid_name = uuid.uuid4() + return 'Original content: {name!s}'.format(name=uuid_name).encode() + + +@pytest.fixture +def dst_exists_path(file_paths_pair, other_payload): + """Return a data destination path.""" + path = file_paths_pair[1] + path.write_bytes(other_payload) + assert path.exists() + return path + + def test_make_sftp(sftp_session): """Smoke-test SFTP instance creation.""" assert sftp_session @@ -67,6 +83,12 @@ def test_get(dst_path, src_path, sftp_session, transmit_payload): assert dst_path.read_bytes() == transmit_payload +def test_get_existing(dst_exists_path, src_path, sftp_session, transmit_payload): + """Check that SFTP file download works when target file exists.""" + sftp_session.get(str(src_path), str(dst_exists_path)) + assert dst_exists_path.read_bytes() == transmit_payload + + @pytest.fixture def large_payload(): """Generate a large 1025 byte (1024 + 1B) test payload.""" From 9d105eb7961e0632d1e667b22d9c201cb33f1368 Mon Sep 17 00:00:00 2001 From: Jakub Jelen <jjelen@redhat.com> Date: Fri, 15 Nov 2024 14:11:09 +0100 Subject: [PATCH 6/8] tests: Symetric test for upload to override existing file Signed-off-by: Jakub Jelen <jjelen@redhat.com> --- tests/unit/sftp_test.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/unit/sftp_test.py b/tests/unit/sftp_test.py index 93446bd9f..86ee4c1f1 100644 --- a/tests/unit/sftp_test.py +++ b/tests/unit/sftp_test.py @@ -89,6 +89,12 @@ def test_get_existing(dst_exists_path, src_path, sftp_session, transmit_payload) assert dst_exists_path.read_bytes() == transmit_payload +def test_put_existing(dst_exists_path, src_path, sftp_session, transmit_payload): + """Check that SFTP file upload works when target file exists.""" + sftp_session.put(str(src_path), str(dst_exists_path)) + assert dst_exists_path.read_bytes() == transmit_payload + + @pytest.fixture def large_payload(): """Generate a large 1025 byte (1024 + 1B) test payload.""" From c612b5a5e3e1ef4cc667f4e0ff8c6527eef7f5ef Mon Sep 17 00:00:00 2001 From: Jakub Jelen <jjelen@redhat.com> Date: Thu, 23 Jan 2025 19:19:17 +0100 Subject: [PATCH 7/8] tests: Simplify generating payload Signed-off-by: Jakub Jelen <jjelen@redhat.com> --- tests/unit/sftp_test.py | 52 ++++++++++++----------------------------- 1 file changed, 15 insertions(+), 37 deletions(-) diff --git a/tests/unit/sftp_test.py b/tests/unit/sftp_test.py index 86ee4c1f1..d9c3c8d41 100644 --- a/tests/unit/sftp_test.py +++ b/tests/unit/sftp_test.py @@ -20,11 +20,21 @@ def sftp_session(ssh_client_session): del sftp_sess # noqa: WPS420 -@pytest.fixture -def transmit_payload(): - """Generate a binary test payload.""" - uuid_name = uuid.uuid4() - return 'Hello, {name!s}'.format(name=uuid_name).encode() +@pytest.fixture( + params=(32, 1024 + 1), + ids=('small-payload', 'large-payload'), +) +def transmit_payload(request: pytest.FixtureRequest): + """Generate binary test payloads of assorted sizes. + + The choice 32 is arbitrary small value. + + The choice 1024 + 1 is meant to be 1B larger than the chunk size used in + sftp.pyx to make sure we excercise at least two rounds of reading/writing. + """ + payload_len = request.param + random_bytes = [ord(random.choice(string.printable)) for _ in range(payload_len)] + return bytes(random_bytes) @pytest.fixture @@ -93,35 +103,3 @@ def test_put_existing(dst_exists_path, src_path, sftp_session, transmit_payload) """Check that SFTP file upload works when target file exists.""" sftp_session.put(str(src_path), str(dst_exists_path)) assert dst_exists_path.read_bytes() == transmit_payload - - -@pytest.fixture -def large_payload(): - """Generate a large 1025 byte (1024 + 1B) test payload.""" - payload_len = 1024 + 1 - random_bytes = [ord(random.choice(string.printable)) for _ in range(payload_len)] - return bytes(random_bytes) - - -@pytest.fixture -def src_path_large(tmp_path, large_payload): - """Return a remote path to a 1025 byte-sized file. - - The pylibssh chunk size is 1024 so the test needs a file that would - execute at least two loops. - """ - path = tmp_path / 'large.txt' - path.write_bytes(large_payload) - return path - - -def test_put_large(dst_path, src_path_large, sftp_session, large_payload): - """Check that SFTP can upload large file.""" - sftp_session.put(str(src_path_large), str(dst_path)) - assert dst_path.read_bytes() == large_payload - - -def test_get_large(dst_path, src_path_large, sftp_session, large_payload): - """Check that SFTP can download large file.""" - sftp_session.get(str(src_path_large), str(dst_path)) - assert dst_path.read_bytes() == large_payload From 565bb942f5246d3c180da5d70aad6775669f5144 Mon Sep 17 00:00:00 2001 From: Jakub Jelen <jjelen@redhat.com> Date: Fri, 30 Aug 2024 13:21:44 +0200 Subject: [PATCH 8/8] Add history fragment Signed-off-by: Jakub Jelen <jjelen@redhat.com> --- docs/changelog-fragments/638.bugfix.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog-fragments/638.bugfix.rst diff --git a/docs/changelog-fragments/638.bugfix.rst b/docs/changelog-fragments/638.bugfix.rst new file mode 100644 index 000000000..fd8973725 --- /dev/null +++ b/docs/changelog-fragments/638.bugfix.rst @@ -0,0 +1,5 @@ +Fixed reading files over SFTP that go over the pre-defined chunk size. + +Prior to this change, the files could end up being corrupted, ending up with the last read chunk written to the file instead of the entire payload. + +-- by :user:`Jakuje`.