Re: [libssh2] libssh2_sftp_close_handle: how to handle errors

Dan Fandrich <dan_at_coneharvesters.com> writes:
> On Thu, Apr 24, 2008 at 08:35:34PM -0700, J.T. Conklin wrote:
>> libssh2_sftp_close_handle() currently returns -1 on failure. What is
>> client code supposed to do to handle this?
>
> Good question. If the failure is due to an out of memory condition, then
> trying again when more memory is available is probably the right thing.
> But in other cases, like if the socket is already closed, then there's
> nothing you can do. Problem is, some resources aren't freed in the latter
> case, putting you into a catch-22 situation. That should really be fixed
> (somehow) in the library.

Sorry all, it has taken a while for this to reach the top of my queue
again.

As I said in my original message, having libssh2_sftp_close_handle()
return errors on failures due to a closed socket, etc. results in an
infinate loop in libssh2_sftp_dtor.

For our product, I changed libssh2_sftp_close_handle() to do a "best
effort" close, which ensures all resources are closed and cleaned up
(to the greatest extent possible). This is similar to other protocol
libraries, eg. OpenLDAP's ldap_unbind() function. We've been using
this change for the last few months without issues (in the same
hostile network environment that used to regularly trigger the
failure).

I checked the current CVS version of sftp.c, and it looks like more
effort has been made to make libssh2_sftp_close_handle() return
PACKET_EAGAIN when an underlying libssh_* operation fails with
PACKET_EAGAIN. I'm not sure what benefit this is supposed to provide.
It seems to push error handling for nonblocking connections up to the
client, so what was a "libssh2_sftp_close_handle(h);" has to be
changed to "while(libssh2_sftp_close_handle(h) != PACKET_EAGAIN);".

I really think this libssh2_sftp_close_handle() needs to be changed
to be best effort and blocking, like ldap_unbind(). Otherwise sftp
in practice can not be used reliabily. After looking at the bugs
filed @ SF, I think this is the same issue as reported as 1940276.

    --jtc

FWIW, this is the patch we're using against a CVS snapshot taken on
2007/05/16.

Index: src/sftp.c
===================================================================
--- src/sftp.c (revision 11094)
+++ src/sftp.c (working copy)
@@ -1502,13 +1502,15 @@
         unsigned long data_len, retcode, request_id;
         ssize_t packet_len = handle->handle_len + 13; /* packet_len(4) + packet_type(1) + request_id(4) + handle_len(4) */
         unsigned char *packet, *s, *data;
- int rc;
+ int rc;
+ int retval = 0;
 
         _libssh2_debug(session, LIBSSH2_DBG_SFTP, "Closing handle");
         s = packet = LIBSSH2_ALLOC(session, packet_len);
         if (!packet) {
                 libssh2_error(session, LIBSSH2_ERROR_ALLOC, "Unable to allocate memory for FXP_CLOSE packet", 0);
- return -1;
+ retval = -1;
+ goto error;
         }
 
         libssh2_htonu32(s, packet_len - 4); s += 4;
@@ -1521,7 +1523,8 @@
         if (packet_len != libssh2_channel_write(channel, (char *)packet, packet_len)) {
                 libssh2_error(session, LIBSSH2_ERROR_SOCKET_SEND, "Unable to send FXP_CLOSE command", 0);
                 LIBSSH2_FREE(session, packet);
- return -1;
+ retval = -1;
+ goto error;
         }
         LIBSSH2_FREE(session, packet);
 
@@ -1531,7 +1534,8 @@
                 }
         if (rc) {
                 libssh2_error(session, LIBSSH2_ERROR_SOCKET_TIMEOUT, "Timeout waiting for status message", 0);
- return -1;
+ retval = -1;
+ goto error;
         }
 
         retcode = libssh2_ntohu32(data + 5);
@@ -1540,9 +1544,10 @@
         if (retcode != LIBSSH2_FX_OK) {
                 sftp->last_errno = retcode;
                 libssh2_error(session, LIBSSH2_ERROR_SFTP_PROTOCOL, "SFTP Protocol Error", 0);
- return -1;
+ retval = -1;
         }
 
+error:
         if (handle == sftp->handles) {
                 sftp->handles = handle->next;
         }
@@ -1558,7 +1563,7 @@
         LIBSSH2_FREE(session, handle->handle);
         LIBSSH2_FREE(session, handle);
 
- return 0;
+ return retval;
 }
 /* }}} */
 

-- 
J.T. Conklin
-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08
_______________________________________________
libssh2-devel mailing list
libssh2-devel_at_lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libssh2-devel