openssh unix dev - May 2025 - can function sftp_upload return OK even if an error message is received?

Hi,

With reference to the latest version of the portable OpenSSH, in file
sftp-client.c, it looks to me there may be a bug in function sftp_upload.

My understanding is that, when variable "len" is equal to 0, no more
SSH_FXP_WRITE messages are sent out and you start draining the queue of pending
responses. Variable "len" is set to 0 either when the upload is
interrupted, or when the SSH_FXP_STATUS response message carries an error, which
sets variable "status" to a value different from SSH_FX_OK.

Can the variable "status" be overwritten by subsequent response
messages to be again SSH_FX_OK? And, if this is the case, isn't it that the
"read" is again called, variable "len" is set again to a
non-zero value, and thus the function can return 0 even if an error message was
received?

Kind regards,
Graziano Stefani.

On Tue, 13 May 2025, Graziano Stefani (Nokia) via openssh-unix-dev wrote:
> Hi,
>
> With reference to the latest version of the portable OpenSSH, in
> file sftp-client.c, it looks to me there may be a bug in function
> sftp_upload.
>
> My understanding is that, when variable "len" is equal to 0, no
more
> SSH_FXP_WRITE messages are sent out and you start draining the queue
> of pending responses. Variable "len" is set to 0 either when the
> upload is interrupted, or when the SSH_FXP_STATUS response message
> carries an error, which sets variable "status" to a value
different
> from SSH_FX_OK.
>
> Can the variable "status" be overwritten by subsequent response
> messages to be again SSH_FX_OK? And, if this is the case, isn't it
> that the "read" is again called, variable "len" is set
again to a
> non-zero value, and thus the function can return 0 even if an error
> message was received?
Thanks, I'm not 100% sure it can happen but that's alone enough reason
to make it perfectly obvious that it can't.

diff --git a/sftp-client.c b/sftp-client.c
index f80352f..964fb3b 100644
--- a/sftp-client.c
+++ b/sftp-client.c
@@ -2010,7 +2010,7 @@ sftp_upload(struct sftp_conn *conn, const char
*local_path,
     int fsync_flag, int inplace_flag)
 {
 	int r, local_fd;
-	u_int openmode, id, status = SSH2_FX_OK, reordered = 0;
+	u_int openmode, id, status = SSH2_FX_OK, failed = 0, reordered = 0;
 	off_t offset, progress_counter;
 	u_char type, *handle, *data;
 	struct sshbuf *msg;
@@ -2150,6 +2150,8 @@ sftp_upload(struct sftp_conn *conn, const char
*local_path,
 			if ((r = sshbuf_get_u32(msg, &status)) != 0)
 				fatal_fr(r, "parse status");
 			debug3("SSH2_FXP_STATUS %u", status);
+			if (status != SSH2_FX_OK)
+				failed = 1;
 
 			/* Find the request in our queue */
 			if ((ack = request_find(&acks, rid)) == NULL)
@@ -2219,7 +2221,7 @@ sftp_upload(struct sftp_conn *conn, const char
*local_path,
 
 	free(handle);
 
-	return status == SSH2_FX_OK ? 0 : -1;
+	return status != SSH2_FX_OK || failed ? -1 : 0;
 }
 
 static int