From 798ec47b57e9c3e701488b8a359e5a7b1dcbdc5f Mon Sep 17 00:00:00 2001 From: Damien Miller Date: Sun, 30 Apr 2023 22:54:23 +0000 Subject: adjust ftruncate() logic to handle servers that reorder requests. sftp/scp will ftruncate the destination file after a transfer completes, to deal with the case where a longer destination file already existed. We tracked the highest contiguous block transferred to deal with this case, but our naive tracking doesn't deal with servers that reorder requests - a misfeature strictly permitted by the protocol but seldom implemented. Adjust the logic to ftruncate() at the highest absolute block received when the transfer is successful. feedback deraadt@ ok markus@ prompted by https://github.com/openssh/openssh-portable/commit/9b733#commitcomment-110679778 --- usr.bin/ssh/sftp-client.c | 50 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 11 deletions(-) (limited to 'usr.bin') diff --git a/usr.bin/ssh/sftp-client.c b/usr.bin/ssh/sftp-client.c index 8b9c50c656e..230e8791e0b 100644 --- a/usr.bin/ssh/sftp-client.c +++ b/usr.bin/ssh/sftp-client.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sftp-client.c,v 1.170 2023/03/28 07:44:32 dtucker Exp $ */ +/* $OpenBSD: sftp-client.c,v 1.171 2023/04/30 22:54:22 djm Exp $ */ /* * Copyright (c) 2001-2004 Damien Miller * @@ -1580,7 +1580,7 @@ do_download(struct sftp_conn *conn, const char *remote_path, u_char *handle; int local_fd = -1, write_error; int read_error, write_errno, lmodified = 0, reordered = 0, r; - u_int64_t offset = 0, size, highwater; + u_int64_t offset = 0, size, highwater = 0, maxack = 0; u_int mode, id, buflen, num_req, max_req, status = SSH2_FX_OK; off_t progress_counter; size_t handle_len; @@ -1627,7 +1627,6 @@ do_download(struct sftp_conn *conn, const char *remote_path, error("open local \"%s\": %s", local_path, strerror(errno)); goto fail; } - offset = highwater = 0; if (resume_flag) { if (fstat(local_fd, &st) == -1) { error("stat local \"%s\": %s", @@ -1648,7 +1647,7 @@ do_download(struct sftp_conn *conn, const char *remote_path, close(local_fd); return -1; } - offset = highwater = st.st_size; + offset = highwater = maxack = st.st_size; } /* Read from remote and write to local */ @@ -1730,11 +1729,21 @@ do_download(struct sftp_conn *conn, const char *remote_path, write_errno = errno; write_error = 1; max_req = 0; + } else { + /* + * Track both the highest offset acknowledged + * and the highest *contiguous* offset + * acknowledged. + * We'll need the latter for ftruncate()ing + * interrupted transfers. + */ + if (maxack < req->offset + len) + maxack = req->offset + len; + if (!reordered && req->offset <= highwater) + highwater = maxack; + else if (!reordered && req->offset > highwater) + reordered = 1; } - else if (!reordered && req->offset <= highwater) - highwater = req->offset + len; - else if (!reordered && req->offset > highwater) - reordered = 1; progress_counter += len; free(data); @@ -1783,12 +1792,19 @@ do_download(struct sftp_conn *conn, const char *remote_path, /* Sanity check */ if (TAILQ_FIRST(&requests) != NULL) fatal("Transfer complete, but requests still in queue"); + + if (!read_error && !write_error && !interrupted) { + /* we got everything */ + highwater = maxack; + } + /* * Truncate at highest contiguous point to avoid holes on interrupt, * or unconditionally if writing in place. */ if (inplace_flag || read_error || write_error || interrupted) { - if (reordered && resume_flag) { + if (reordered && resume_flag && + (read_error || write_error || interrupted)) { error("Unable to resume download of \"%s\": " "server reordered requests", local_path); } @@ -1984,7 +2000,7 @@ do_upload(struct sftp_conn *conn, const char *local_path, struct stat sb; Attrib a, t, *c = NULL; u_int32_t startid, ackid; - u_int64_t highwater = 0; + u_int64_t highwater = 0, maxack = 0; struct request *ack = NULL; struct requests acks; size_t handle_len; @@ -2125,8 +2141,16 @@ do_upload(struct sftp_conn *conn, const char *local_path, ack->id, ack->len, (unsigned long long)ack->offset); ++ackid; progress_counter += ack->len; + /* + * Track both the highest offset acknowledged and the + * highest *contiguous* offset acknowledged. + * We'll need the latter for ftruncate()ing + * interrupted transfers. + */ + if (maxack < ack->offset + ack->len) + maxack = ack->offset + ack->len; if (!reordered && ack->offset <= highwater) - highwater = ack->offset + ack->len; + highwater = maxack; else if (!reordered && ack->offset > highwater) { debug3_f("server reordered ACKs"); reordered = 1; @@ -2143,6 +2167,10 @@ do_upload(struct sftp_conn *conn, const char *local_path, stop_progress_meter(); free(data); + if (status == SSH2_FX_OK && !interrupted) { + /* we got everything */ + highwater = maxack; + } if (status != SSH2_FX_OK) { error("write remote \"%s\": %s", remote_path, fx2txt(status)); status = SSH2_FX_FAILURE; -- cgit v1.2.3