libssh2_publickey_init: fixed to work better non-blocking

This was triggered by a clang-analyzer complaint that turned out
to be valid, and it made me dig deeper and fix some generic non-
blocking problems I disovered in the code.

While cleaning this up, I moved session-specific stuff over to a
new session.h header from the libssh2_priv.h header.

src/channel.c

@@ -50,6 +50,7 @@
 #include "channel.h"
 #include "transport.h"
 #include "packet.h"
+#include "session.h"
 
 /*
  *  _libssh2_channel_nextid

src/libssh2_priv.h

@@ -857,6 +857,9 @@ struct _LIBSSH2_SESSION
     LIBSSH2_CHANNEL *pkeyInit_channel;
     unsigned char *pkeyInit_data;
     size_t pkeyInit_data_len;
+    /* 19 = packet_len(4) + version_len(4) + "version"(7) + version_num(4) */
+    unsigned char pkeyInit_buffer[19];
+    size_t pkeyInit_buffer_sent; /* how much of buffer that has been sent */
 
     /* State variables used in libssh2_packet_add() */
     libssh2_nonblocking_states packAdd_state;
@@ -1107,8 +1110,6 @@ _libssh2_debug(LIBSSH2_SESSION * session, int context, const char *format, ...)
 #define SSH_MSG_CHANNEL_SUCCESS                     99
 #define SSH_MSG_CHANNEL_FAILURE                     100
 
-void _libssh2_session_shutdown(LIBSSH2_SESSION * session);
-
 #ifdef WIN32
 ssize_t _libssh2_recv(libssh2_socket_t socket, void *buffer, size_t length, int flags);
 ssize_t _libssh2_send(libssh2_socket_t socket, const void *buffer, size_t length, int flags);
@@ -1120,15 +1121,10 @@ ssize_t _libssh2_send(libssh2_socket_t socket, const void *buffer, size_t length
 #define LIBSSH2_READ_TIMEOUT 60 /* generic timeout in seconds used when
                                    waiting for more data to arrive */
 
-int _libssh2_wait_socket(LIBSSH2_SESSION *session);
-
 
 int libssh2_kex_exchange(LIBSSH2_SESSION * session, int reexchange,
                          key_exchange_state_t * state);
 
-/* this is the lib-internal set blocking function */
-int _libssh2_session_set_blocking(LIBSSH2_SESSION * session, int blocking);
-
 /* Let crypt.c/hostkey.c expose their method structs */
 const LIBSSH2_CRYPT_METHOD **libssh2_crypt_methods(void);
 const LIBSSH2_HOSTKEY_METHOD **libssh2_hostkey_methods(void);
@@ -1148,49 +1144,6 @@ int _libssh2_pem_decode_integer(unsigned char **data, unsigned int *datalen,
 /* global.c */
 void _libssh2_init_if_needed (void);
 
-/* Conveniance-macros to allow code like this;
-
-   int rc = BLOCK_ADJUST(rc, session, session_startup(session, sock) );
-
-   int rc = BLOCK_ADJUST_ERRNO(ptr, session, session_startup(session, sock) );
-
-   The point of course being to make sure that while in non-blocking mode
-   these always return no matter what the return code is, but in blocking mode
-   it blocks if EAGAIN is the reason for the return from the underlying
-   function.
-
-*/
-#define BLOCK_ADJUST(rc,sess,x) \
-    do { \
-       rc = x; \
-       /* the order of the check below is important to properly deal with the
-          case when the 'sess' is freed */ \
-       if((rc != LIBSSH2_ERROR_EAGAIN) || !sess->api_block_mode)  \
-           break; \
-       rc = _libssh2_wait_socket(sess); \
-       if(rc) \
-           break; \
-    } while(1)
-
-/*
- * For functions that returns a pointer, we need to check if the API is
- * non-blocking and return immediately. If the pointer is non-NULL we return
- * immediately. If the API is blocking and we get a NULL we check the errno
- * and *only* if that is EAGAIN we loop and wait for socket action.
- */
-#define BLOCK_ADJUST_ERRNO(ptr,sess,x)          \
-    do { \
-       int rc; \
-       ptr = x; \
-       if(!sess->api_block_mode || \
-          (ptr != NULL) || \
-          (libssh2_session_last_errno(sess) != LIBSSH2_ERROR_EAGAIN) ) \
-           break;                                                  \
-       rc = _libssh2_wait_socket(sess); \
-       if(rc) \
-           break; \
-    } while(1)
-
 
 #define ARRAY_SIZE(a) (sizeof ((a)) / sizeof ((a)[0]))
 

src/publickey.c

@@ -1,4 +1,5 @@
 /* Copyright (c) 2004-2007, Sara Golemon <sarag@libssh2.org>
+ * Copyright (c) 2010 by Daniel Stenberg
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms,
@@ -38,6 +39,7 @@
 #include "libssh2_priv.h"
 #include "libssh2_publickey.h"
 #include "channel.h"
+#include "session.h"
 
 #define LIBSSH2_PUBLICKEY_VERSION               2
 
@@ -215,7 +217,7 @@ publickey_response_id(unsigned char **pdata, size_t data_len)
     return -1;
 }
 
-/* libssh2_publickey_response_success
+/* publickey_response_success
  *
  * Generic helper routine to wait for success response and nothing else
  */
@@ -226,10 +228,9 @@ publickey_response_success(LIBSSH2_PUBLICKEY * pkey)
     unsigned char *data, *s;
     size_t data_len;
     int response;
-    int rc;
 
     while (1) {
-        rc = publickey_packet_receive(pkey, &data, &data_len);
+        int rc = publickey_packet_receive(pkey, &data, &data_len);
         if (rc == LIBSSH2_ERROR_EAGAIN) {
             return rc;
         } else if (rc) {
@@ -239,11 +240,7 @@ publickey_response_success(LIBSSH2_PUBLICKEY * pkey)
         }
 
         s = data;
-        if ((response = publickey_response_id(&s, data_len)) < 0) {
+        response = publickey_response_id(&s, data_len);
-            LIBSSH2_FREE(session, data);
-            return _libssh2_error(session, LIBSSH2_ERROR_PUBLICKEY_PROTOCOL,
-                                  "Invalid publickey subsystem response code");
-        }
 
         switch (response) {
         case LIBSSH2_PUBLICKEY_RESPONSE_STATUS:
@@ -260,10 +257,14 @@ publickey_response_success(LIBSSH2_PUBLICKEY * pkey)
             return -1;
         }
         default:
+            LIBSSH2_FREE(session, data);
+            if (response < 0) {
+                return _libssh2_error(session, LIBSSH2_ERROR_PUBLICKEY_PROTOCOL,
+                                      "Invalid publickey subsystem response code");
+            }
             /* Unknown/Unexpected */
             _libssh2_error(session, LIBSSH2_ERROR_PUBLICKEY_PROTOCOL,
                            "Unexpected publickey subsystem response, ignoring");
-            LIBSSH2_FREE(session, data);
             data = NULL;
         }
     }
@@ -276,16 +277,12 @@ publickey_response_success(LIBSSH2_PUBLICKEY * pkey)
  ***************** */
 
 /*
- * libssh2_publickey_init
+ * publickey_init
  *
  * Startup the publickey subsystem
  */
-LIBSSH2_API LIBSSH2_PUBLICKEY *
+static LIBSSH2_PUBLICKEY *publickey_init(LIBSSH2_SESSION *session)
-libssh2_publickey_init(LIBSSH2_SESSION * session)
 {
-    /* 19 = packet_len(4) + version_len(4) + "version"(7) + version_num(4) */
-    unsigned char buffer[19];
-    unsigned char *s;
     int response;
     int rc;
 
@@ -301,37 +298,30 @@ libssh2_publickey_init(LIBSSH2_SESSION * session)
     }
 
     if (session->pkeyInit_state == libssh2_NB_state_allocated) {
-        do {
+
-            session->pkeyInit_channel =
+        session->pkeyInit_channel =
-                libssh2_channel_open_ex(session, "session",
+            _libssh2_channel_open(session, "session",
-                                        sizeof("session") - 1,
+                                  sizeof("session") - 1,
-                                        LIBSSH2_CHANNEL_WINDOW_DEFAULT,
+                                  LIBSSH2_CHANNEL_WINDOW_DEFAULT,
-                                        LIBSSH2_CHANNEL_PACKET_DEFAULT, NULL,
+                                  LIBSSH2_CHANNEL_PACKET_DEFAULT, NULL,
-                                        0);
+                                  0);
-            if (!session->pkeyInit_channel
+        if (!session->pkeyInit_channel) {
-                && (libssh2_session_last_errno(session) ==
+            if (libssh2_session_last_errno(session) == LIBSSH2_ERROR_EAGAIN)
-                    LIBSSH2_ERROR_EAGAIN)) {
                 /* The error state is already set, so leave it */
-                _libssh2_error(session, LIBSSH2_ERROR_EAGAIN,
-                               "Would block to startup channel");
                 return NULL;
-            } else if (!session->pkeyInit_channel
+            _libssh2_error(session, LIBSSH2_ERROR_CHANNEL_FAILURE,
-                       && (libssh2_session_last_errno(session) !=
+                           "Unable to startup channel");
-                           LIBSSH2_ERROR_EAGAIN)) {
+            goto err_exit;
-                _libssh2_error(session, LIBSSH2_ERROR_CHANNEL_FAILURE,
+        }
-                               "Unable to startup channel");
-                goto err_exit;
-            }
-        } while (!session->pkeyInit_channel);
 
         session->pkeyInit_state = libssh2_NB_state_sent;
     }
 
     if (session->pkeyInit_state == libssh2_NB_state_sent) {
-        rc = libssh2_channel_process_startup(session->pkeyInit_channel,
+        rc = _libssh2_channel_process_startup(session->pkeyInit_channel,
-                                             "subsystem",
+                                              "subsystem",
-                                             sizeof("subsystem") - 1,
+                                              sizeof("subsystem") - 1,
-                                             "publickey", strlen("publickey"));
+                                              "publickey", strlen("publickey"));
         if (rc == LIBSSH2_ERROR_EAGAIN) {
             _libssh2_error(session, LIBSSH2_ERROR_EAGAIN,
                            "Would block starting publickey subsystem");
@@ -346,8 +336,9 @@ libssh2_publickey_init(LIBSSH2_SESSION * session)
     }
 
     if (session->pkeyInit_state == libssh2_NB_state_sent1) {
-        rc = libssh2_channel_handle_extended_data2(session->pkeyInit_channel,
+        unsigned char *s;
-                                                   LIBSSH2_CHANNEL_EXTENDED_DATA_IGNORE);
+        rc = _libssh2_channel_extended_data(session->pkeyInit_channel,
+                                            LIBSSH2_CHANNEL_EXTENDED_DATA_IGNORE);
         if (rc == LIBSSH2_ERROR_EAGAIN) {
             _libssh2_error(session, LIBSSH2_ERROR_EAGAIN,
                            "Would block starting publickey subsystem");
@@ -365,7 +356,7 @@ libssh2_publickey_init(LIBSSH2_SESSION * session)
         session->pkeyInit_pkey->channel = session->pkeyInit_channel;
         session->pkeyInit_pkey->version = 0;
 
-        s = buffer;
+        s = session->pkeyInit_buffer;
         _libssh2_htonu32(s, 4 + (sizeof("version") - 1) + 4);
         s += 4;
         _libssh2_htonu32(s, sizeof("version") - 1);
@@ -374,6 +365,7 @@ libssh2_publickey_init(LIBSSH2_SESSION * session)
         s += sizeof("version") - 1;
         _libssh2_htonu32(s, LIBSSH2_PUBLICKEY_VERSION);
         s += 4;
+        session->pkeyInit_buffer_sent = 0;
 
         _libssh2_debug(session, LIBSSH2_TRACE_PUBLICKEY,
                        "Sending publickey version packet advertising version %d support",
@@ -384,22 +376,30 @@ libssh2_publickey_init(LIBSSH2_SESSION * session)
 
     if (session->pkeyInit_state == libssh2_NB_state_sent2) {
         rc = _libssh2_channel_write(session->pkeyInit_channel, 0,
-                                    (char *) buffer, (s - buffer));
+                                    (char *)session->pkeyInit_buffer,
+                                    19 - session->pkeyInit_buffer_sent);
         if (rc == LIBSSH2_ERROR_EAGAIN) {
             _libssh2_error(session, LIBSSH2_ERROR_EAGAIN,
                            "Would block sending publickey version packet");
             return NULL;
-        } else if ((s - buffer) != rc) {
+        } else if (rc) {
-            _libssh2_error(session, LIBSSH2_ERROR_SOCKET_SEND,
+            _libssh2_error(session, rc,
                            "Unable to send publickey version packet");
             goto err_exit;
         }
+        session->pkeyInit_buffer_sent += rc;
+        if(session->pkeyInit_buffer_sent < 19) {
+            _libssh2_error(session, LIBSSH2_ERROR_EAGAIN,
+                           "Need to be called again to complete this");
+            return NULL;
+        }
 
         session->pkeyInit_state = libssh2_NB_state_sent3;
     }
 
     if (session->pkeyInit_state == libssh2_NB_state_sent3) {
         while (1) {
+            unsigned char *s;
             rc = publickey_packet_receive(session->pkeyInit_pkey,
                                           &session->pkeyInit_data,
                                           &session->pkeyInit_data_len);
@@ -506,6 +506,23 @@ libssh2_publickey_init(LIBSSH2_SESSION * session)
     return NULL;
 }
 
+/*
+ * libssh2_publickey_init
+ *
+ * Startup the publickey subsystem
+ */
+LIBSSH2_API LIBSSH2_PUBLICKEY *
+libssh2_publickey_init(LIBSSH2_SESSION *session)
+{
+    LIBSSH2_PUBLICKEY *ptr;
+
+    BLOCK_ADJUST_ERRNO(ptr, session,
+                       publickey_init(session));
+    return ptr;
+}
+
+
+
 /*
  * libssh2_publickey_add_ex
  *

src/scp.c

@@ -1,4 +1,4 @@
-/* Copyright (c) 2009 by Daniel Stenberg
+/* Copyright (c) 2009-2010 by Daniel Stenberg
  * Copyright (c) 2004-2008, Sara Golemon <sarag@libssh2.org>
  * All rights reserved.
  *
@@ -41,6 +41,7 @@
 #include <stdlib.h>
 
 #include "channel.h"
+#include "session.h"
 
 
 /* Max. length of a quoted string after libssh2_shell_quotearg() processing */

src/session.c

@@ -1,5 +1,5 @@
 /* Copyright (c) 2004-2007 Sara Golemon <sarag@libssh2.org>
- * Copyright (c) 2009 by Daniel Stenberg
+ * Copyright (c) 2009-2010 by Daniel Stenberg
  * Copyright (c) 2010 Simon Josefsson <simon@josefsson.org>
  * All rights reserved.
  *
@@ -53,6 +53,7 @@
 #endif
 
 #include "transport.h"
+#include "session.h"
 
 /* libssh2_default_alloc
  */
@@ -520,8 +521,8 @@ libssh2_session_callback_set(LIBSSH2_SESSION * session,
 int _libssh2_wait_socket(LIBSSH2_SESSION *session)
 {
     int rc;
-    int dir;
     int seconds_to_next;
+    int dir;
 
     rc = libssh2_keepalive_send (session, &seconds_to_next);
     if (rc < 0)

src/session.h

@@ -0,0 +1,91 @@
+#ifndef LIBSSH2_SESSION_H
+#define LIBSSH2_SESSION_H
+/* Copyright (c) 2004-2007 Sara Golemon <sarag@libssh2.org>
+ * Copyright (c) 2009-2010 by Daniel Stenberg
+ * Copyright (c) 2010 Simon Josefsson <simon@josefsson.org>
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms,
+ * with or without modification, are permitted provided
+ * that the following conditions are met:
+ *
+ *   Redistributions of source code must retain the above
+ *   copyright notice, this list of conditions and the
+ *   following disclaimer.
+ *
+ *   Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials
+ *   provided with the distribution.
+ *
+ *   Neither the name of the copyright holder nor the names
+ *   of any other contributors may be used to endorse or
+ *   promote products derived from this software without
+ *   specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+ * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
+ * USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY
+ * OF SUCH DAMAGE.
+ */
+
+/* Conveniance-macros to allow code like this;
+
+   int rc = BLOCK_ADJUST(rc, session, session_startup(session, sock) );
+
+   int rc = BLOCK_ADJUST_ERRNO(ptr, session, session_startup(session, sock) );
+
+   The point of course being to make sure that while in non-blocking mode
+   these always return no matter what the return code is, but in blocking mode
+   it blocks if EAGAIN is the reason for the return from the underlying
+   function.
+
+*/
+#define BLOCK_ADJUST(rc,sess,x) \
+    do { \
+       rc = x; \
+       /* the order of the check below is important to properly deal with the
+          case when the 'sess' is freed */ \
+       if((rc != LIBSSH2_ERROR_EAGAIN) || !sess->api_block_mode)  \
+           break; \
+       rc = _libssh2_wait_socket(sess); \
+       if(rc) \
+           break; \
+    } while(1)
+
+/*
+ * For functions that returns a pointer, we need to check if the API is
+ * non-blocking and return immediately. If the pointer is non-NULL we return
+ * immediately. If the API is blocking and we get a NULL we check the errno
+ * and *only* if that is EAGAIN we loop and wait for socket action.
+ */
+#define BLOCK_ADJUST_ERRNO(ptr,sess,x)          \
+    do { \
+       int rc; \
+       ptr = x; \
+       if(!sess->api_block_mode || \
+          (ptr != NULL) || \
+          (libssh2_session_last_errno(sess) != LIBSSH2_ERROR_EAGAIN) ) \
+           break;                                                  \
+       rc = _libssh2_wait_socket(sess); \
+       if(rc) \
+           break; \
+    } while(1)
+
+
+int _libssh2_wait_socket(LIBSSH2_SESSION *session);
+
+/* this is the lib-internal set blocking function */
+int _libssh2_session_set_blocking(LIBSSH2_SESSION * session, int blocking);
+
+#endif /* LIBSSH2_SESSION_H */

src/sftp.c

@@ -41,6 +41,7 @@
 #include "libssh2_priv.h"
 #include "libssh2_sftp.h"
 #include "channel.h"
+#include "session.h"
 
 /* Note: Version 6 was documented at the time of writing
  * However it was marked as "DO NOT IMPLEMENT" due to pending changes

src/userauth.c

@@ -49,6 +49,7 @@
 #endif
 
 #include "transport.h"
+#include "session.h"
 
 /* libssh2_userauth_list
  *