[PATCH] sftp: Avoid race condition reading incomplete data messages

[Jakub Jelen <jjelen@xxxxxxxxxx>]

Hi list,

This changes amends f561e6bcb361999088fe377f750dbacbc5e0102f which
introduces same check in one place, but miss it in other two places.

We encountered this issue with qemu using SFTP to transfer large
data chunks and in some cases, the file transfer was interrupted
without any reason. From the debug messages, it showed up that
last part of data message/packet was not handled in the time
of the sftp_read() call, therefore the ssh_channel_read() returned
zero (there was no more data to read yet), which made the whole
transfer fail hard instead of retrying later.

The proposed change is reusing the code from previously referenced
commit also in the other places.

Regards,
-- 
Jakub Jelen
Software Engineer
Security Technologies
Red Hat, Inc.

From 1bb06a42430d0f8165dbfbf0e68fbcce51b6e20e Mon Sep 17 00:00:00 2001
From: Jakub Jelen <jjelen@xxxxxxxxxx>
Date: Sun, 26 Aug 2018 21:38:06 +0200
Subject: [PATCH] sftp: Avoid race condition reading incomplete data messages

This changes amends f561e6bcb361999088fe377f750dbacbc5e0102f which
introduces same check in one place, but miss it in other two places.

We encountered this issue with qemu using SFTP to transfer large
data chunks and in some cases, the file transfer was interrupted
without any reason. From the debug messages, it showed up that
last part of data message/packet was not handled in the time
of the sftp_read() call, therefore the ssh_channel_read() returned
zero (there was no more data to read yet), which made the whole
transfer fail hard instead of retrying later.

The proposed change is reusing the code from previously referenced
commit also in the other places.

Signed-off-by: Jakub Jelen <jjelen@xxxxxxxxxx>
---
 src/sftp.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/src/sftp.c b/src/sftp.c
index 2144cb03..87b6ff94 100644
--- a/src/sftp.c
+++ b/src/sftp.c
@@ -303,7 +303,7 @@ sftp_packet sftp_packet_read(sftp_session sftp) {
   sftp_packet packet = NULL;
   uint32_t tmp;
   size_t size;
-  int r, s;
+  int r, s, is_eof;
 
   packet = calloc(1, sizeof(struct sftp_packet_struct));
   if (packet == NULL) {
@@ -330,8 +330,6 @@ sftp_packet sftp_packet_read(sftp_session sftp) {
     if (s < 0) {
         goto error;
     } else if (s == 0) {
-        int is_eof;
-
         is_eof = ssh_channel_is_eof(sftp->channel);
         if (is_eof) {
             goto error;
@@ -346,11 +344,17 @@ sftp_packet sftp_packet_read(sftp_session sftp) {
     goto error;
   }
 
-  r=ssh_channel_read(sftp->channel, buffer, 1, 0);
-  if (r <= 0) {
-    /* TODO: check if there are cases where an error needs to be set here */
-    goto error;
-  }
+  do {
+    r = ssh_channel_read(sftp->channel, buffer, 1, 0);
+    if (r < 0) {
+        goto error;
+    } else if (s == 0) {
+        is_eof = ssh_channel_is_eof(sftp->channel);
+        if (is_eof) {
+            goto error;
+        }
+    }
+  } while (r < 1);
   ssh_buffer_add_data(packet->payload, buffer, r);
   ssh_buffer_get_u8(packet->payload, &packet->type);
 
@@ -369,11 +373,16 @@ sftp_packet sftp_packet_read(sftp_session sftp) {
     r=ssh_channel_read(sftp->channel,buffer,
         sizeof(buffer)>size ? size:sizeof(buffer),0);
 
-    if(r <= 0) {
+    if (r < 0) {
       /* TODO: check if there are cases where an error needs to be set here */
       goto error;
-    }
-    if (ssh_buffer_add_data(packet->payload, buffer, r) == SSH_ERROR) {
+    } else if (r == 0) {
+      /* Retry the reading unless the remote was closed */
+      is_eof = ssh_channel_is_eof(sftp->channel);
+      if (is_eof) {
+          goto error;
+      }
+    } else if (ssh_buffer_add_data(packet->payload, buffer, r) == SSH_ERROR) {
       ssh_set_error_oom(sftp->session);
       goto error;
     }
-- 
2.17.1