Graziano Stefani (Nokia)
2025-Sep-19 14:49 UTC
FW: can function sftp_upload return OK even if an error message is received?
Hi, Were there any conclusions on this subject? Can the proposed fix (introducing variable status_2 to save the error in the SSH2_FXP_STATUS message) be acceptable? Kind regards, Graziano. -----Original Message----- From: Graziano Stefani (Nokia) Sent: Tuesday, May 20, 2025 1:59 PM To: Damien Miller <djm at mindrot.org> Cc: openssh-unix-dev at mindrot.org Subject: RE: can function sftp_upload return OK even if an error message is received? Hi, Provided that condition "(status == SSH2_FX_OK && !interrupted)" indicates everything went okay, can we 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 should also 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. 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
Damien Miller
2025-Sep-29 23:25 UTC
FW: can function sftp_upload return OK even if an error message is received?
On Fri, 19 Sep 2025, Graziano Stefani (Nokia) via openssh-unix-dev wrote:> Hi, > > Were there any conclusions on this subject? Can the proposed > fix (introducing variable status_2 to save the error in the > SSH2_FXP_STATUS message) be acceptable?> - debug3("SSH2_FXP_STATUS %u", status); > + debug3("SSH2_FXP_STATUS %u", status_2); > + if (status == SSH2_FX_OK) > + status = status_2;I think this patch is wrong. The condition should be != SSH2_FX_OK, otherwise the error is never stored. diff --git a/sftp-client.c b/sftp-client.c index 0993291..004cbe3 100644 --- a/sftp-client.c +++ b/sftp-client.c @@ -2018,7 +2018,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, status2, reordered = 0; off_t offset, progress_counter; u_char type, *handle, *data; struct sshbuf *msg; @@ -2155,9 +2155,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, &status2)) != 0) fatal_fr(r, "parse status"); - debug3("SSH2_FXP_STATUS %u", status); + debug3("SSH2_FXP_STATUS %u", status2); + if (status2 != SSH2_FX_OK) + status = status2; /* remember errors */ /* Find the request in our queue */ if ((ack = request_find(&acks, rid)) == NULL)