diff --git a/src/ssh/sftp_client.cpp b/src/ssh/sftp_client.cpp index 823b2c72d0..404e19b192 100644 --- a/src/ssh/sftp_client.cpp +++ b/src/ssh/sftp_client.cpp @@ -30,7 +30,6 @@ #include constexpr int file_mode = 0664; -constexpr auto max_transfer = 65536u; const std::string stream_file_name{"stream_output.dat"}; const char* log_category = "sftp"; @@ -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 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 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)}; } @@ -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 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 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); } } diff --git a/tests/test_sftp_client.cpp b/tests/test_sftp_client.cpp index e501f51891..89b8f5363c 100644 --- a/tests/test_sftp_client.cpp +++ b/tests/test_sftp_client.cpp @@ -77,10 +77,19 @@ struct SFTPClient : public testing::Test return {std::make_unique("b", 43, "ubuntu", key_provider)}; } +// this is a macro since REPLACE only applies to the current scope and cannot be moved out. +#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 sftp_new; MockScope free_sftp; + sftp_limits_struct limits{32768, 32768, 32768, 0}; + const mpt::StubSSHKeyProvider key_provider; mpt::MockSSHTestFixture mock_ssh_test_fixture; @@ -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(); @@ -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, _)) @@ -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; @@ -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())); @@ -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, _)) @@ -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)); @@ -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, _)) @@ -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; @@ -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())); @@ -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; @@ -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(); @@ -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())); @@ -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())); REPLACE(sftp_open, [](auto sftp, auto...) { return get_dummy_sftp_file(sftp); }); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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) { @@ -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(); @@ -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(); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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)); @@ -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();