GNOME.org
 

[PATCH] Improved sftp I/O error handling

The attached patch ensures that buffer_recv and buffer_send errors are
propagated to their parents. While hangs may still occur, crashes
shouldn't happen anymore.

-- 
Christian Neumair <chris gnome-de org>

Index: modules/sftp-method.c
===================================================================
--- modules/sftp-method.c	(Revision 5338)
+++ modules/sftp-method.c	(Arbeitskopie)
@@ -305,7 +305,7 @@ buffer_write (Buffer *buf, gconstpointer
 	buf->write_ptr += size;
 }
 
-static GnomeVFSResult
+static G_GNUC_WARN_UNUSED_RESULT GnomeVFSResult
 buffer_send (Buffer *buf, int fd) 
 {
 	guint bytes_written = 0;
@@ -343,7 +343,7 @@ buffer_send (Buffer *buf, int fd) 
 	return res;
 }
 
-static GnomeVFSResult
+static G_GNUC_WARN_UNUSED_RESULT GnomeVFSResult
 buffer_recv (Buffer *buf, int fd) 
 {
 	guint32 r_len, len, bytes_read;
@@ -673,14 +673,19 @@ sftp_status_to_vfs_result (guint status)
 
 /* Derived from OpenSSH, sftp-client.c:get_status */
 
-static GnomeVFSResult
+static G_GNUC_WARN_UNUSED_RESULT GnomeVFSResult
 iobuf_read_result (int fd, guint expected_id)
 {
 	Buffer msg;
+	GnomeVFSResult res;
 	guint type, id, status;
 
 	buffer_init (&msg);
-	buffer_recv (&msg, fd);
+	res = buffer_recv (&msg, fd);
+	if (res != GNOME_VFS_OK) {
+		return res;
+	}
+
 	type = buffer_read_gchar (&msg);
 	id = buffer_read_gint32 (&msg);
 
@@ -699,15 +704,19 @@ iobuf_read_result (int fd, guint expecte
 
 /* Derived from OpenSSH, sftp-client.c:get_handle */
 
-static GnomeVFSResult
+static G_GNUC_WARN_UNUSED_RESULT GnomeVFSResult
 iobuf_read_handle (int fd, gchar **handle, guint expected_id, guint32 *len)
 {
 	Buffer msg;
+	GnomeVFSResult res;
 	gchar type;
 	guint id, status;
 
 	buffer_init (&msg);
-	buffer_recv (&msg, fd);
+	res = buffer_recv (&msg, fd);
+	if (res != GNOME_VFS_OK) {
+		return res;
+	}
 
 	type = buffer_read_gchar (&msg);
 	id = buffer_read_gint32 (&msg);
@@ -736,7 +745,7 @@ iobuf_read_handle (int fd, gchar **handl
 
 /* this neither includes the name nor the MIME type,
  * which are set in update_mime_type_and_name_from_path */
-static GnomeVFSResult
+static G_GNUC_WARN_UNUSED_RESULT GnomeVFSResult
 iobuf_read_file_info (int fd, GnomeVFSFileInfo *info, guint expected_id)
 {
 	Buffer msg;
@@ -745,7 +754,10 @@ iobuf_read_file_info (int fd, GnomeVFSFi
 	GnomeVFSResult res;
 
 	buffer_init (&msg);
-	buffer_recv (&msg, fd);
+	res = buffer_recv (&msg, fd);
+	if (res != GNOME_VFS_OK) {
+		return res;
+	}
 
 	type = buffer_read_gchar (&msg);
 	id = buffer_read_gint32 (&msg);
@@ -775,7 +787,7 @@ iobuf_read_file_info (int fd, GnomeVFSFi
 
 /* Derived from OpenSSH, sftp-client.c:send_read_request */
 
-static GnomeVFSResult
+static G_GNUC_WARN_UNUSED_RESULT GnomeVFSResult
 iobuf_send_read_request (int            fd,
 			 guint          id,
 			 guint64        offset,
@@ -802,7 +814,7 @@ iobuf_send_read_request (int            
 
 /* Derived from OpenSSH, sftp-client.c:send_string_request */
 
-static void
+static G_GNUC_WARN_UNUSED_RESULT GnomeVFSResult
 iobuf_send_string_request (int            fd,
 			   guint          id,
 			   guint          code,
@@ -810,20 +822,24 @@ iobuf_send_string_request (int          
 			   guint          len)
 {
 	Buffer msg;
+	GnomeVFSResult res;
 
 	buffer_init (&msg);
 
 	buffer_write_gchar (&msg, code);
 	buffer_write_gint32 (&msg, id);
 	buffer_write_block (&msg, s, len);
-	buffer_send (&msg, fd);
+	res = buffer_send (&msg, fd);
 
 	buffer_free (&msg);
+
+	return res;
 }
 
 /* Derived from OpenSSH, sftp-client.c:send_string_attrs_request */
 
-static void
+
+static G_GNUC_WARN_UNUSED_RESULT GnomeVFSResult
 iobuf_send_string_request_with_file_info (int                      fd,
 					  guint                    id,
 					  guint                    code,
@@ -833,6 +849,7 @@ iobuf_send_string_request_with_file_info
 					  GnomeVFSSetFileInfoMask  mask)
 {
 	Buffer msg;
+	GnomeVFSResult res;
 
 	buffer_init (&msg);
 
@@ -840,9 +857,11 @@ iobuf_send_string_request_with_file_info
 	buffer_write_gint32 (&msg, id);
 	buffer_write_block (&msg, s, len);
 	buffer_write_file_info (&msg, info, mask);
-	buffer_send (&msg, fd);
+	res = buffer_send (&msg, fd);
 
 	buffer_free (&msg);
+
+	return res;
 }
 
 static char*
@@ -1232,7 +1251,10 @@ tty_retry:
 
 	buffer_write_gchar (&msg, SSH2_FXP_INIT);
 	buffer_write_gint32 (&msg, SSH2_FILEXFER_VERSION);
-	buffer_send (&msg, out_fd);
+	res = buffer_send (&msg, out_fd);
+	if (res != GNOME_VFS_OK) {
+		goto bail;
+	}
 
 	tty_channel = NULL;
 	if (tty_fd != -1) {
@@ -1897,10 +1919,12 @@ do_open (GnomeVFSMethod        *method,
 	memset (&info, 0, sizeof (GnomeVFSFileInfo));
 	buffer_write_file_info (&msg, &info, GNOME_VFS_SET_FILE_INFO_NONE);
 
-	buffer_send (&msg, conn->out_fd);
+	res = buffer_send (&msg, conn->out_fd);
 	buffer_free (&msg);
-	
-	res = iobuf_read_handle (conn->in_fd, &sftp_handle, id, (guint32 *)&sftp_handle_len);
+
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_handle (conn->in_fd, &sftp_handle, id, (guint32 *)&sftp_handle_len);
+	}
 
 	if (res == GNOME_VFS_OK) {
 		handle = g_new0 (SftpOpenHandle, 1);
@@ -1982,10 +2006,12 @@ do_create (GnomeVFSMethod        *method
 	info.permissions = perm;
 	buffer_write_file_info (&msg, &info, GNOME_VFS_SET_FILE_INFO_PERMISSIONS);
 
-	buffer_send (&msg, conn->out_fd);
+	res = buffer_send (&msg, conn->out_fd);
 	buffer_free (&msg);
 
-	res = iobuf_read_handle (conn->in_fd, &sftp_handle, id, &sftp_handle_len);
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_handle (conn->in_fd, &sftp_handle, id, &sftp_handle_len);
+	}
 
 	if (res == GNOME_VFS_OK) {
 		handle = g_new0 (SftpOpenHandle, 1);
@@ -2019,10 +2045,9 @@ do_close (GnomeVFSMethod       *method,
 {
 	SftpOpenHandle *handle;
 
-	guint id, status;
+	guint i, id;
 	Buffer msg;
-
-	guint i;
+	GnomeVFSResult res;
 
 	DEBUG (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG, "%s: Enter", G_GNUC_FUNCTION));
 
@@ -2036,9 +2061,11 @@ do_close (GnomeVFSMethod       *method,
 	buffer_write_gchar (&msg, SSH2_FXP_CLOSE);
 	buffer_write_gint32 (&msg, id);
 	buffer_write_block (&msg, handle->sftp_handle, handle->sftp_handle_len);
-	buffer_send (&msg, handle->connection->out_fd);
+	res = buffer_send (&msg, handle->connection->out_fd);
 
-	status = iobuf_read_result (handle->connection->in_fd, id);
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_result (handle->connection->in_fd, id);
+	}
 
 	buffer_free (&msg);
 	sftp_connection_unref (handle->connection);
@@ -2054,7 +2081,7 @@ do_close (GnomeVFSMethod       *method,
 
 	DEBUG (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG, "%s: Exit", G_GNUC_FUNCTION));
 
-	return status;
+	return res;
 }
 
 static GnomeVFSResult 
@@ -2124,20 +2151,25 @@ do_read (GnomeVFSMethod       *method, 
 				      read_req[req_ptr].req_len, read_req[req_ptr].ptr));
 
 			outstanding++;
-			iobuf_send_read_request (handle->connection->out_fd,
+			result = iobuf_send_read_request (handle->connection->out_fd,
 						 read_req[req_ptr].id,
 						 handle->offset + (read_req[req_ptr].ptr - buffer),
 						 read_req[req_ptr].req_len,
 						 handle->sftp_handle, handle->sftp_handle_len);
+			if (result != GNOME_VFS_OK) {
+				break;
+			}
 
 			curr_ptr += read_req[req_ptr].req_len;
 
 			req_ptr = (req_ptr + 1) % queue_len;
 		}
 
-		buffer_clear (&msg);
-		result = buffer_recv (&msg, handle->connection->in_fd);
-		outstanding--;
+		if (result == GNOME_VFS_OK) {
+			buffer_clear (&msg);
+			result = buffer_recv (&msg, handle->connection->in_fd);
+			outstanding--;
+		}
 
 		if (result != GNOME_VFS_OK) {
 			buffer_free (&msg);
@@ -2203,18 +2235,27 @@ do_read (GnomeVFSMethod       *method, 
 			*bytes_read += len;
 
 			if (len < read_req[req_svc].req_len) {
+				GnomeVFSResult result;
+
 				/* Missing data. Request that part */
 				read_req[req_svc].id = sftp_connection_get_id (handle->connection);
 				read_req[req_svc].req_len -= len;
 				read_req[req_svc].ptr += len;
 
-				outstanding++;
-				iobuf_send_read_request
+				result = iobuf_send_read_request
 					(handle->connection->out_fd,
 					 read_req[req_svc].id,
 					 handle->offset + (read_req[req_svc].ptr - buffer),
 					 read_req[req_svc].req_len,
 					 handle->sftp_handle, handle->sftp_handle_len);
+				if (result != GNOME_VFS_OK) {
+					buffer_free (&msg);
+					g_free (read_req);
+					sftp_connection_unlock (handle->connection);
+					return GNOME_VFS_ERROR_PROTOCOL_ERROR;
+				}
+
+				outstanding++;
 			} else
 				read_req[req_svc].id = 0;
 
@@ -2293,6 +2334,8 @@ do_write (GnomeVFSMethod       *method, 
 	sftp_connection_lock (handle->connection);
 
 	while (*bytes_written < num_bytes) {
+		GnomeVFSResult res = GNOME_VFS_OK;
+
 		while (curr_offset < num_bytes &&
 		       (req_ptr + 1) % queue_len != req_svc_ptr) {
 			write_req[req_ptr].id = sftp_connection_get_id (handle->connection);
@@ -2314,13 +2357,24 @@ do_write (GnomeVFSMethod       *method, 
 			buffer_write_block (&msg, buffer + write_req[req_ptr].offset,
 					    write_req[req_ptr].req_len);
 			
-			buffer_send (&msg, handle->connection->out_fd);
+			res = buffer_send (&msg, handle->connection->out_fd);
 
 			req_ptr = (req_ptr + 1) % queue_len;
 		}
 
 		buffer_clear (&msg);
-		buffer_recv (&msg, handle->connection->in_fd);
+
+		if (res == GNOME_VFS_OK) {
+			res = buffer_recv (&msg, handle->connection->in_fd);
+		}
+
+		if (res != GNOME_VFS_OK) {
+			buffer_free (&msg);
+			g_free (write_req);
+			sftp_connection_unlock (handle->connection);
+			return res;
+		}
+
 		type = buffer_read_gchar (&msg);
 		recv_id = buffer_read_gint32 (&msg);
 
@@ -2445,6 +2499,7 @@ sftp_readlink (SftpConnection *connectio
 	char type;
 	unsigned int id;
 	char *ret;
+	GnomeVFSResult res;
 
 	DEBUG (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG, "%s: Enter", G_GNUC_FUNCTION));
 
@@ -2453,21 +2508,27 @@ sftp_readlink (SftpConnection *connectio
 	DEBUG (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG, "%s: Requesting symlink target for %s",
 		      G_GNUC_FUNCTION, path));
 
+	ret = NULL;
+
 	buffer_init (&msg);
 	buffer_write_gchar (&msg, SSH2_FXP_READLINK);
 	buffer_write_gint32 (&msg, id);
 	buffer_write_string (&msg, path);
-	buffer_send (&msg, connection->out_fd);
+	res = buffer_send (&msg, connection->out_fd);
+	if (res != GNOME_VFS_OK) {
+		goto out;
+	}
 
 	buffer_clear (&msg);
 
-	buffer_recv (&msg, connection->in_fd);
+	res = buffer_recv (&msg, connection->in_fd);
+	if (res != GNOME_VFS_OK) {
+		goto out;
+	}
 
 	type = buffer_read_gchar (&msg);
 	recv_id = buffer_read_gint32 (&msg);
 
-	ret = NULL;
-
 	if (recv_id != id)
 		g_critical ("%s: ID mismatch (%u != %u)", G_GNUC_FUNCTION, recv_id, id);
 	else if (type == SSH2_FXP_NAME) {
@@ -2494,6 +2555,7 @@ sftp_readlink (SftpConnection *connectio
 			      G_GNUC_FUNCTION, type));
 	}
 
+out:
 	buffer_free (&msg);
 
 	return ret;
@@ -2559,9 +2621,12 @@ get_file_info_for_path (SftpConnection  
 
 	id = sftp_connection_get_id (conn);
 
-	iobuf_send_string_request (conn->out_fd, id,
+	res = iobuf_send_string_request (conn->out_fd, id,
 				   SSH2_FXP_LSTAT,
 				   path, strlen (path));
+	if (res != GNOME_VFS_OK) {
+		return res;
+	}
 
 	res = iobuf_read_file_info (conn->in_fd, file_info, id);
 	if (res != GNOME_VFS_OK) return res;
@@ -2617,11 +2682,14 @@ get_file_info_for_path (SftpConnection  
 			g_free (tmp);
 
 			id = sftp_connection_get_id (conn);
-			iobuf_send_string_request (conn->out_fd, id,
+			res = iobuf_send_string_request (conn->out_fd, id,
 						   SSH2_FXP_LSTAT,
 						   target_path, strlen (target_path));
 
-			res = iobuf_read_file_info (conn->in_fd, target_info, id);
+			if (res == GNOME_VFS_OK) {
+				res = iobuf_read_file_info (conn->in_fd, target_info, id);
+			}
+
 			if (res != GNOME_VFS_OK ||
 			    (target_info->valid_fields & GNOME_VFS_FILE_INFO_FIELDS_TYPE) == 0) {
 				DEBUG (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG,
@@ -2761,11 +2829,13 @@ do_open_directory (GnomeVFSMethod       
 	buffer_write_gchar (&msg, SSH2_FXP_OPENDIR);
 	buffer_write_gint32 (&msg, id);
 	buffer_write_string (&msg, path);
-	buffer_send (&msg, conn->out_fd);
+	res = buffer_send (&msg, conn->out_fd);
 
 	buffer_free (&msg);
 
-	res = iobuf_read_handle (conn->in_fd, &sftp_handle, id, &sftp_handle_len);
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_handle (conn->in_fd, &sftp_handle, id, &sftp_handle_len);
+	}
 
 	DEBUG (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG, "%s: Result is %d", G_GNUC_FUNCTION, res));
 
@@ -2823,6 +2893,7 @@ do_read_directory (GnomeVFSMethod       
 	gint status, count, i;
 	Buffer msg;
 	gchar type;
+	GnomeVFSResult res;
 
 	DEBUG (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG, "%s: Enter", G_GNUC_FUNCTION));
 
@@ -2850,11 +2921,17 @@ do_read_directory (GnomeVFSMethod       
 	buffer_write_gchar (&msg, SSH2_FXP_READDIR);
 	buffer_write_gint32 (&msg, id);
 	buffer_write_block (&msg, handle->sftp_handle, handle->sftp_handle_len);
-	buffer_send (&msg, handle->connection->out_fd);
+	res = buffer_send (&msg, handle->connection->out_fd);
 
 	buffer_clear (&msg);
 
-	buffer_recv (&msg, handle->connection->in_fd);
+	if (res == GNOME_VFS_OK) {
+		res = buffer_recv (&msg, handle->connection->in_fd);
+	}
+
+	if (res != GNOME_VFS_OK) {
+		return res;
+	}
 
 	type = buffer_read_gchar (&msg);
 	recv_id = buffer_read_gint32 (&msg);
@@ -2994,13 +3071,15 @@ do_make_directory (GnomeVFSMethod  *meth
 	URI_TO_PATH (uri, path);
 
 	memset (&info, 0, sizeof (GnomeVFSFileInfo));
-	iobuf_send_string_request_with_file_info (conn->out_fd, id, SSH2_FXP_MKDIR,
+	res = iobuf_send_string_request_with_file_info (conn->out_fd, id, SSH2_FXP_MKDIR,
 						  path, strlen (path), &info,
 						  GNOME_VFS_SET_FILE_INFO_NONE);
 
 	g_free (path);
 
-	res = iobuf_read_result (conn->in_fd, id);
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_result (conn->in_fd, id);
+	}
 
 	sftp_connection_unref (conn);
 	sftp_connection_unlock (conn);
@@ -3029,10 +3108,12 @@ do_remove_directory (GnomeVFSMethod  *me
 	id = sftp_connection_get_id (conn);
 
 	URI_TO_PATH (uri, path);
-	iobuf_send_string_request (conn->out_fd, id, SSH2_FXP_RMDIR, path, strlen (path));
+	res = iobuf_send_string_request (conn->out_fd, id, SSH2_FXP_RMDIR, path, strlen (path));
 	g_free (path);
 
-	res = iobuf_read_result (conn->in_fd, id);
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_result (conn->in_fd, id);
+	}
 
 	sftp_connection_unref (conn);
 	sftp_connection_unlock (conn);
@@ -3072,12 +3153,16 @@ do_move (GnomeVFSMethod  *method,
 
 	/* if force_replace is specified, try to remove new_uri */
 	if (force_replace) {
-		iobuf_send_string_request (conn->out_fd,
+		res = iobuf_send_string_request (conn->out_fd,
 					   id,
 					   SSH2_FXP_REMOVE,
 					   new_path,
 					   strlen (new_path));
-		res = iobuf_read_result (conn->in_fd, id);
+
+		if (res == GNOME_VFS_OK) {
+			res = iobuf_read_result (conn->in_fd, id);
+		}
+
 		if (res != GNOME_VFS_OK && res != GNOME_VFS_ERROR_NOT_FOUND)
 			goto bail;
 	}
@@ -3087,10 +3172,12 @@ do_move (GnomeVFSMethod  *method,
 	buffer_write_gint32 (&msg, id);
 	buffer_write_string (&msg, old_path);
 	buffer_write_string (&msg, new_path);
-	buffer_send (&msg, conn->out_fd);
+	res = buffer_send (&msg, conn->out_fd);
 	buffer_free (&msg);
 
-	res = iobuf_read_result (conn->in_fd, id);
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_result (conn->in_fd, id);
+	}
 
  bail:
 	g_free (old_path);
@@ -3141,13 +3228,15 @@ do_rename (GnomeVFSMethod  *method,
 	buffer_write_gint32 (&msg, id);
 	buffer_write_string (&msg, old_path);
 	buffer_write_string (&msg, new_path);
-	buffer_send (&msg, conn->out_fd);
+	res = buffer_send (&msg, conn->out_fd);
 	buffer_free (&msg);
 
 	g_free (old_path);
 	g_free (new_path);
 
-	res = iobuf_read_result (conn->in_fd, id);
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_result (conn->in_fd, id);
+	}
 
 	sftp_connection_unref (conn);
 	sftp_connection_unlock (conn);
@@ -3171,10 +3260,12 @@ do_unlink (GnomeVFSMethod  *method,
 	id = sftp_connection_get_id (conn);
 
 	URI_TO_PATH (uri, path);
-	iobuf_send_string_request (conn->out_fd, id, SSH2_FXP_REMOVE, path, strlen (path));
+	res = iobuf_send_string_request (conn->out_fd, id, SSH2_FXP_REMOVE, path, strlen (path));
 	g_free (path);
 
-	res = iobuf_read_result (conn->in_fd, id);
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_result (conn->in_fd, id);
+	}
 
 	sftp_connection_unref (conn);
 	sftp_connection_unlock (conn);
@@ -3257,11 +3348,13 @@ do_set_file_info (GnomeVFSMethod        
 		id = sftp_connection_get_id (conn);
 
 		URI_TO_PATH (uri, path);
-		iobuf_send_string_request_with_file_info (conn->out_fd, id, SSH2_FXP_SETSTAT,
-							  path, strlen (path), info, mask);
+		res = iobuf_send_string_request_with_file_info (conn->out_fd, id, SSH2_FXP_SETSTAT,
+								path, strlen (path), info, mask);
 		g_free (path);
 
-		res = iobuf_read_result (conn->in_fd, id);
+		if (res == GNOME_VFS_OK) {
+			res = iobuf_read_result (conn->in_fd, id);
+		}
 
 		sftp_connection_unref (conn);
 		sftp_connection_unlock (conn);
@@ -3329,10 +3422,12 @@ do_create_symlink (GnomeVFSMethod   *met
 	buffer_write_gint32 (&msg, id);
 	buffer_write_string (&msg, real_target);
 	buffer_write_string (&msg, path);
-	buffer_send (&msg, conn->out_fd);
+	res = buffer_send (&msg, conn->out_fd);
 	buffer_free (&msg);
 
-	res = iobuf_read_result (conn->in_fd, id);
+	if (res == GNOME_VFS_OK) {
+		res = iobuf_read_result (conn->in_fd, id);
+	}
 
 	sftp_connection_unref (conn);
 	sftp_connection_unlock (conn);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]