can function sftp_upload return OK even if an error message is received?

[Graziano Stefani (Nokia)]

Hi,

Provided that condition "(status == SSH2_FX_OK && !interrupted)" indicates everything went okay, I would do something like:

diff --git a/sftp-client.c b/sftp-client.c
index 9f8ab4afa..6c029127d 100644
--- a/sftp-client.c
+++ b/sftp-client.c
@@ -2034,7 +2034,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_2, status = SSH2_FX_OK, reordered = 0;
        off_t offset, progress_counter;
        u_char type, *handle, *data;
        struct sshbuf *msg;
@@ -2172,9 +2172,11 @@ sftp_upload(struct sftp_conn *conn, const char *local_path,
                                fatal("Expected SSH2_FXP_STATUS(%d) packet, "
                                    "got %d", SSH2_FXP_STATUS, type);

-                       if ((r = sshbuf_get_u32(msg, &status)) != 0)
+                       if ((r = sshbuf_get_u32(msg, &status_2)) != 0)
                                fatal_fr(r, "parse status");
-                       debug3("SSH2_FXP_STATUS %u", status);
+                       debug3("SSH2_FXP_STATUS %u", status_2);
+                       if (status == SSH2_FX_OK)
+                               status = status_2;

                        /* Find the request in our queue */
                        if ((ack = request_find(&acks, rid)) == NULL)

this also should guarantee that "len" is set to 0 when "status != SSH2_FX_OK" and never returns to non-zero value.

In other words, stop sending out messages and wait for the queue of pending responses to drain before returning...unless this is not the intended behavior.

Kind regards,
Graziano.

-----Original Message-----
From: Damien Miller <djm at mindrot.org> 
Sent: Tuesday, May 20, 2025 10:09 AM
To: Graziano Stefani (Nokia) <graziano.stefani at nokia.com>
Cc: openssh-unix-dev at mindrot.org
Subject: Re: can function sftp_upload return OK even if an error message is received?

[You don't often get email from djm at mindrot.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.



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