From 155a002fc906a567591c1b3737aed95faad44c38 Mon Sep 17 00:00:00 2001
From: djm <djm@openbsd.org>
Date: Sun, 30 Apr 2023 22:54:22 +0000
Subject: [PATCH] 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(-)

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 <djm@openbsd.org>
  *
@@ -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;
-- 
2.20.1