Skip to content

Fix transfer only partially transferring #3902

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ricab merged 3 commits into from
Feb 7, 2025
Merged

Fix transfer only partially transferring #3902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
495f6c2
[sftp] Use sftp_limits for stfp buffer sizes
Sploder12 Jan 27, 2025
dca86e4
Fix tests to init limits
Sploder12 Jan 29, 2025
3d9d0e6
Use uninitialized buffer
Sploder12 Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions src/ssh/sftp_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#include <fmt/std.h>

constexpr int file_mode = 0664;
constexpr auto max_transfer = 65536u;
const std::string stream_file_name{"stream_output.dat"};
const char* log_category = "sftp";

Expand Down Expand Up @@ -331,9 +330,12 @@ void SFTPClient::do_push_file(std::istream& source, const fs::path& target_path)
if (!remote_file)
throw SFTPError{"cannot open remote file {}: {}", target_path, ssh_get_error(sftp->session)};

std::array<char, max_transfer> buffer{};
while (auto r = source.read(buffer.data(), buffer.size()).gcount())
if (sftp_write(remote_file.get(), buffer.data(), r) < 0)
// create an uninitialized buffer to use.
const auto max_write = sftp_limits(sftp.get())->max_write_length;
const std::unique_ptr<char[]> buffer{new char[max_write]};

while (auto r = source.read(buffer.get(), max_write).gcount())
if (sftp_write(remote_file.get(), buffer.get(), r) < 0)
throw SFTPError{"cannot write to remote file {}: {}", target_path, ssh_get_error(sftp->session)};
}

Expand All @@ -343,13 +345,16 @@ void SFTPClient::do_pull_file(const fs::path& source_path, std::ostream& target)
if (!remote_file)
throw SFTPError{"cannot open remote file {}: {}", source_path, ssh_get_error(sftp->session)};

std::array<char, max_transfer> buffer{};
while (auto r = sftp_read(remote_file.get(), buffer.data(), buffer.size()))
// create an uninitialized buffer to use.
const auto max_read = sftp_limits(sftp.get())->max_read_length;
const std::unique_ptr<char[]> buffer{new char[max_read]};

while (auto r = sftp_read(remote_file.get(), buffer.get(), max_read))
{
if (r < 0)
throw SFTPError{"cannot read from remote file {}: {}", source_path, ssh_get_error(sftp->session)};

target.write(buffer.data(), r);
target.write(buffer.get(), r);
}
}

Expand Down
75 changes: 42 additions & 33 deletions tests/test_sftp_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,19 @@ struct SFTPClient : public testing::Test
return {std::make_unique<mp::SSHSession>("b", 43, "ubuntu", key_provider)};
}

// this is a macro since REPLACE only applies to the current scope and cannot be moved out.
Copy link
Copy Markdown
Collaborator

@ricab ricab Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you need the capture so extracting the lambda wouldn't work. You'd need std::bind or another lambda again. Alright.

#define REPLACE_SFTP_INIT() \
REPLACE(sftp_init, [this](sftp_session sftp) { \
sftp->limits = &limits; \
return SSH_OK; \
});

decltype(MOCK(sftp_close)) close{MOCK(sftp_close)};
MockScope<decltype(mock_sftp_new)> sftp_new;
MockScope<decltype(mock_sftp_free)> free_sftp;

sftp_limits_struct limits{32768, 32768, 32768, 0};

const mpt::StubSSHKeyProvider key_provider;
mpt::MockSSHTestFixture mock_ssh_test_fixture;

Expand Down Expand Up @@ -115,7 +124,7 @@ TEST_F(SFTPClient, throws_when_failed_to_init)

TEST_F(SFTPClient, is_dir)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();

auto mocked_sftp_stat = MOCK(sftp_stat);
auto sftp_client = make_sftp_client();
Expand All @@ -132,7 +141,7 @@ TEST_F(SFTPClient, push_file_success)
{
std::string test_data = "test_data";

REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(false));
EXPECT_CALL(*mock_sftp_utils, get_remote_file_target(_, source_path, target_path, _)).WillOnce(Return(target_path));
EXPECT_CALL(*mock_file_ops, open_read(source_path, _))
Expand All @@ -159,7 +168,7 @@ TEST_F(SFTPClient, push_file_success)

TEST_F(SFTPClient, push_file_cannot_open_source)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(false));
EXPECT_CALL(*mock_sftp_utils, get_remote_file_target(_, source_path, target_path, _)).WillOnce(Return(target_path));
auto err = EACCES;
Expand All @@ -179,7 +188,7 @@ TEST_F(SFTPClient, push_file_cannot_open_source)

TEST_F(SFTPClient, push_file_cannot_open_target)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(false));
EXPECT_CALL(*mock_sftp_utils, get_remote_file_target(_, source_path, target_path, _)).WillOnce(Return(target_path));
EXPECT_CALL(*mock_file_ops, open_read(source_path, _)).WillOnce(Return(std::make_unique<std::stringstream>()));
Expand All @@ -197,7 +206,7 @@ TEST_F(SFTPClient, push_file_cannot_write_target)
{
std::string test_data = "test_data";

REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(false));
EXPECT_CALL(*mock_sftp_utils, get_remote_file_target(_, source_path, target_path, _)).WillOnce(Return(target_path));
EXPECT_CALL(*mock_file_ops, open_read(source_path, _))
Expand All @@ -218,7 +227,7 @@ TEST_F(SFTPClient, push_file_cannot_read_source)
{
std::string test_data = "test_data";

REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(false));
EXPECT_CALL(*mock_sftp_utils, get_remote_file_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -248,7 +257,7 @@ TEST_F(SFTPClient, push_file_cannot_set_perms)
{
std::string test_data = "test_data";

REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(false));
EXPECT_CALL(*mock_sftp_utils, get_remote_file_target(_, source_path, target_path, _)).WillOnce(Return(target_path));
EXPECT_CALL(*mock_file_ops, open_read(source_path, _))
Expand All @@ -274,7 +283,7 @@ TEST_F(SFTPClient, pull_file_success)
{
std::string test_data = "test_data";

REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_sftp_utils, get_local_file_target(source_path, target_path, _)).WillOnce(Return(target_path));

std::stringstream test_file;
Expand Down Expand Up @@ -307,7 +316,7 @@ TEST_F(SFTPClient, pull_file_success)

TEST_F(SFTPClient, pull_file_cannot_open_source)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(); });
EXPECT_CALL(*mock_sftp_utils, get_local_file_target(source_path, target_path, _)).WillOnce(Return(target_path));
EXPECT_CALL(*mock_file_ops, open_write(target_path, _)).WillOnce(Return(std::make_unique<std::stringstream>()));
Expand All @@ -323,7 +332,7 @@ TEST_F(SFTPClient, pull_file_cannot_open_source)

TEST_F(SFTPClient, pull_file_cannot_open_target)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(); });
EXPECT_CALL(*mock_sftp_utils, get_local_file_target(source_path, target_path, _)).WillOnce(Return(target_path));
auto err = EACCES;
Expand All @@ -343,7 +352,7 @@ TEST_F(SFTPClient, pull_file_cannot_open_target)

TEST_F(SFTPClient, pull_file_cannot_write_target)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_sftp_utils, get_local_file_target(source_path, target_path, _)).WillOnce(Return(target_path));

auto test_file = std::make_unique<std::stringstream>();
Expand Down Expand Up @@ -372,7 +381,7 @@ TEST_F(SFTPClient, pull_file_cannot_write_target)

TEST_F(SFTPClient, pull_file_cannot_read_source)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(); });
EXPECT_CALL(*mock_sftp_utils, get_local_file_target(source_path, target_path, _)).WillOnce(Return(target_path));
EXPECT_CALL(*mock_file_ops, open_write(target_path, _)).WillOnce(Return(std::make_unique<std::stringstream>()));
Expand All @@ -390,7 +399,7 @@ TEST_F(SFTPClient, pull_file_cannot_read_source)

TEST_F(SFTPClient, pull_file_cannot_set_perms)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_sftp_utils, get_local_file_target(source_path, target_path, _)).WillOnce(Return(target_path));
EXPECT_CALL(*mock_file_ops, open_write(target_path, _)).WillOnce(Return(std::make_unique<std::stringstream>()));
REPLACE(sftp_open, [](auto sftp, auto...) { return get_dummy_sftp_file(sftp); });
Expand All @@ -410,7 +419,7 @@ TEST_F(SFTPClient, pull_file_cannot_set_perms)

TEST_F(SFTPClient, push_dir_success_regular)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_sftp_utils, get_remote_dir_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -449,7 +458,7 @@ TEST_F(SFTPClient, push_dir_success_regular)

TEST_F(SFTPClient, push_dir_success_dir)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_sftp_utils, get_remote_dir_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -480,7 +489,7 @@ TEST_F(SFTPClient, push_dir_success_dir)

TEST_F(SFTPClient, push_dir_fail_dir)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_sftp_utils, get_remote_dir_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -513,7 +522,7 @@ TEST_F(SFTPClient, push_dir_fail_dir)

TEST_F(SFTPClient, push_dir_success_symlink)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_sftp_utils, get_remote_dir_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -543,7 +552,7 @@ TEST_F(SFTPClient, push_dir_success_symlink)

TEST_F(SFTPClient, push_dir_cannot_read_symlink)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_sftp_utils, get_remote_dir_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -576,7 +585,7 @@ TEST_F(SFTPClient, push_dir_cannot_read_symlink)

TEST_F(SFTPClient, push_dir_cannot_create_symlink)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_sftp_utils, get_remote_dir_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -610,7 +619,7 @@ TEST_F(SFTPClient, push_dir_cannot_create_symlink)

TEST_F(SFTPClient, push_dir_symlink_over_dir)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_sftp_utils, get_remote_dir_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -641,7 +650,7 @@ TEST_F(SFTPClient, push_dir_symlink_over_dir)

TEST_F(SFTPClient, push_dir_unknown_file_type)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_sftp_utils, get_remote_dir_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand All @@ -668,7 +677,7 @@ TEST_F(SFTPClient, push_dir_unknown_file_type)

TEST_F(SFTPClient, push_dir_open_iter_fail)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));
EXPECT_CALL(*mock_sftp_utils, get_remote_dir_target(_, source_path, target_path, _)).WillOnce(Return(target_path));

Expand All @@ -687,7 +696,7 @@ TEST_F(SFTPClient, push_dir_open_iter_fail)

TEST_F(SFTPClient, push_dir_cannot_access_target)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();

auto err = std::make_error_code(std::errc::permission_denied);
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce([&](auto, std::error_code& e) {
Expand All @@ -703,7 +712,7 @@ TEST_F(SFTPClient, push_dir_cannot_access_target)

TEST_F(SFTPClient, push_dir_r_not_specified)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_file_ops, is_directory(source_path, _)).WillOnce(Return(true));

auto sftp_client = make_sftp_client();
Expand All @@ -715,7 +724,7 @@ TEST_F(SFTPClient, push_dir_r_not_specified)

TEST_F(SFTPClient, pull_dir_success_regular)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
EXPECT_CALL(*mock_sftp_utils, get_local_dir_target(source_path, target_path, _)).WillOnce(Return(target_path));

auto iter = std::make_unique<mpt::MockSFTPDirIterator>();
Expand Down Expand Up @@ -767,7 +776,7 @@ TEST_F(SFTPClient, pull_dir_success_regular)

TEST_F(SFTPClient, pull_dir_success_dir)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(SSH_FILEXFER_TYPE_DIRECTORY); });
EXPECT_CALL(*mock_sftp_utils, get_local_dir_target(source_path, target_path, _)).WillOnce(Return(target_path));

Expand All @@ -788,7 +797,7 @@ TEST_F(SFTPClient, pull_dir_success_dir)

TEST_F(SFTPClient, pull_dir_fail_dir)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(SSH_FILEXFER_TYPE_DIRECTORY); });
EXPECT_CALL(*mock_sftp_utils, get_local_dir_target(source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -818,7 +827,7 @@ TEST_F(SFTPClient, pull_dir_fail_dir)

TEST_F(SFTPClient, pull_dir_success_symlink)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(SSH_FILEXFER_TYPE_DIRECTORY); });
EXPECT_CALL(*mock_sftp_utils, get_local_dir_target(source_path, target_path, _)).WillOnce(Return(target_path));

Expand All @@ -843,7 +852,7 @@ TEST_F(SFTPClient, pull_dir_success_symlink)

TEST_F(SFTPClient, pull_dir_cannot_read_symlink)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(SSH_FILEXFER_TYPE_DIRECTORY); });
EXPECT_CALL(*mock_sftp_utils, get_local_dir_target(source_path, target_path, _)).WillOnce(Return(target_path));

Expand All @@ -869,7 +878,7 @@ TEST_F(SFTPClient, pull_dir_cannot_read_symlink)

TEST_F(SFTPClient, pull_dir_cannot_create_symlink)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(SSH_FILEXFER_TYPE_DIRECTORY); });
EXPECT_CALL(*mock_sftp_utils, get_local_dir_target(source_path, target_path, _)).WillOnce(Return(target_path));

Expand Down Expand Up @@ -898,7 +907,7 @@ TEST_F(SFTPClient, pull_dir_cannot_create_symlink)

TEST_F(SFTPClient, pull_dir_symlink_over_dir)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(SSH_FILEXFER_TYPE_DIRECTORY); });
EXPECT_CALL(*mock_sftp_utils, get_local_dir_target(source_path, target_path, _)).WillOnce(Return(target_path));

Expand All @@ -923,7 +932,7 @@ TEST_F(SFTPClient, pull_dir_symlink_over_dir)

TEST_F(SFTPClient, pull_dir_unknown_file_type)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(SSH_FILEXFER_TYPE_DIRECTORY); });
EXPECT_CALL(*mock_sftp_utils, get_local_dir_target(source_path, target_path, _)).WillOnce(Return(target_path));

Expand All @@ -945,7 +954,7 @@ TEST_F(SFTPClient, pull_dir_unknown_file_type)

TEST_F(SFTPClient, pull_dir_r_not_specified)
{
REPLACE(sftp_init, [](auto...) { return SSH_OK; });
REPLACE_SFTP_INIT();
REPLACE(sftp_stat, [](auto...) { return get_dummy_sftp_attr(SSH_FILEXFER_TYPE_DIRECTORY); });

auto sftp_client = make_sftp_client();
Expand Down