Refactoring Channels

1. Remove the now unused states from the accept state list. Update the state machine to finish on ACCEPT_DONE. 2. Replace the exFds set with writeFds. Call worker if writeFds also set for the ssh socket. 3. If the worker returns WS_WANT_WRITE, WS_WANT_READ, or WS_REKEYING, go ahead and call select again then worker. 4. Handle doing SCP or SFTP later after handling the socket. 5. Add some missing "\n" on error logging in the echoserver. 6. In the sftp test script, clean recommendations from shellcheck. Removed the "-e" options from the echos while removing the newlines; echos with newlines get an additional echo. Removed redundant calls to the pwd program when PWD is already set.
ejohnstown · Nov 4, 2024 · 3fc2341 · 3fc2341
1 parent d5bd4e9
commit 3fc2341
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 80 deletions.
diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c
@@ -958,13 +958,14 @@ static int ssh_worker(thread_ctx_t* threadCtx)
         ChildRunning = 1;
 
         while (ChildRunning) {
-            fd_set readFds, exFds;
+            fd_set readFds, writeFds;
             WS_SOCKET_T maxFd;
             int cnt_r, cnt_w = 0;
 
             FD_ZERO(&readFds);
-            FD_ZERO(&exFds);
+            FD_ZERO(&writeFds);
             FD_SET(sshFd, &readFds);
+            FD_SET(sshFd, &writeFds);
             maxFd = sshFd;
 
             if (threadCtx->shellCtx.isConnected) {
@@ -998,17 +999,16 @@ static int ssh_worker(thread_ctx_t* threadCtx)
                 || threadCtx->fwdCbCtx.state == FWD_STATE_DIRECT_CONNECTED)) {
 
                 FD_SET(fwdFd, &readFds);
-                FD_SET(fwdFd, &exFds);
                 if (fwdFd > maxFd)
                     maxFd = fwdFd;
             }
             #endif /* WOLFSSH_FWD */
 
-            rc = select((int)maxFd + 1, &readFds, NULL, &exFds, NULL);
+            rc = select((int)maxFd + 1, &readFds, &writeFds, NULL, NULL);
             if (rc == -1)
                 break;
 
-            if (FD_ISSET(sshFd, &readFds)) {
+            if (FD_ISSET(sshFd, &readFds) || FD_ISSET(sshFd, &writeFds)) {
                 word32 lastChannel = 0;
 
                 /* The following tries to read from the first channel inside
@@ -1018,16 +1018,6 @@ static int ssh_worker(thread_ctx_t* threadCtx)
                    channel. The additional channel is only used with the
                    agent. */
                 cnt_r = wolfSSH_worker(ssh, &lastChannel);
-                #ifdef WOLFSSH_SCP
-                if (threadCtx->doScp) {
-                    return WS_SCP_INIT;
-                }
-                #endif
-                #ifdef WOLFSSH_SFTP
-                if (threadCtx->doSftp) {
-                    return WS_SFTP_COMPLETE;
-                }
-                #endif
                 if (cnt_r < 0) {
                     rc = wolfSSH_get_error(ssh);
                     if (rc == WS_CHAN_RXD) {
@@ -1137,14 +1127,28 @@ static int ssh_worker(thread_ctx_t* threadCtx)
                         #endif
                         continue;
                     }
-                    else if (rc != WS_WANT_READ && rc != WS_REKEYING) {
+                    else if (rc != WS_WANT_READ && rc != WS_WANT_WRITE
+                            && rc != WS_REKEYING) {
                         #ifdef SHELL_DEBUG
                             printf("Break:read sshFd returns %d: errno =%x\n",
                                     cnt_r, errno);
                         #endif
                         break;
                     }
+                    else {
+                        continue;
+                    }
+                }
+                #ifdef WOLFSSH_SCP
+                if (threadCtx->doScp) {
+                    return WS_SCP_INIT;
                 }
+                #endif
+                #ifdef WOLFSSH_SFTP
+                if (threadCtx->doSftp) {
+                    return WS_SFTP_COMPLETE;
+                }
+                #endif
             }
             #ifdef WOLFSSH_SHELL
             if (threadCtx->shellCtx.isConnected && !threadCtx->echo) {
@@ -2537,13 +2541,13 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
 
                 case 'p':
                     if (myoptarg == NULL) {
-                        ES_ERROR("NULL port value");
+                        ES_ERROR("NULL port value\n");
                     }
                     else {
                         port = (word16)atoi(myoptarg);
                         #if !defined(NO_MAIN_DRIVER) || defined(USE_WINDOWS_API)
                             if (port == 0) {
-                                ES_ERROR("port number cannot be 0");
+                                ES_ERROR("port number cannot be 0\n");
                             }
                         #endif
                     }
@@ -2599,7 +2603,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
 
 #ifdef WOLFSSH_TEST_BLOCK
     if (!nonBlock) {
-        ES_ERROR("Use -N when testing forced non blocking");
+        ES_ERROR("Use -N when testing forced non blocking\n");
     }
 #endif
 
@@ -2701,7 +2705,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
             keyLoadBuf = (byte*)WMALLOC(EXAMPLE_KEYLOAD_BUFFER_SZ,
                     NULL, 0);
             if (keyLoadBuf == NULL) {
-                ES_ERROR("Error allocating keyLoadBuf");
+                ES_ERROR("Error allocating keyLoadBuf\n");
             }
         #else
             keyLoadBuf = buf;
@@ -2932,7 +2936,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
                                                          &clientAddrSz);
     #endif
         if (clientFd == -1) {
-            ES_ERROR("tcp accept failed");
+            ES_ERROR("tcp accept failed\n");
         }
 
         if (nonBlock)

diff --git a/scripts/sftp.test b/scripts/sftp.test
@@ -3,55 +3,57 @@
 # sftp local test
 
 no_pid=-1
-server_pid=$no_pid
-ready_file=`pwd`/wolfssh_sftp_ready$$
+server_pid="$no_pid"
+ready_file="$PWD/wolfssh_sftp_ready$$"
 counter=0
 nonblockingOnly=0
 
-[ ! -x ./examples/sftpclient/wolfsftp ] && echo -e "\n\nwolfSFTP client doesn't exist" && exit 1
+[ ! -x ./examples/sftpclient/wolfsftp ] && echo && echo "wolfSFTP client doesn't exist" && exit 1
 
 # test for if the SFTP client works
-./examples/sftpclient/wolfsftp -h | grep NO_WOLFSSH_CLIENT
-if [ $? -eq 0 ]
+./examples/sftpclient/wolfsftp -h | grep -q NO_WOLFSSH_CLIENT
+RESULT=$?
+if [ $RESULT -eq 0 ]
 then
     echo "macro NO_WOLFSSH_CLIENT was used"
     echo "skipping test"
     exit 77
 fi
 
 # test for nonblocking only
-./examples/client/client -h | grep WOLFSSH_TEST_BLOCK
-if [ $? -eq 0 ]
+./examples/client/client -h | grep -q WOLFSSH_TEST_BLOCK
+RESULT=$?
+if [ $RESULT -eq 0 ]
 then
-    echo "macro NO_WOLFSSH_CLIENT was used"
+    echo "macro WOLFSSH_TEST_BLOCK was used"
     nonblockingOnly=1
 fi
 
 #echo "ready file $ready_file"
 
 create_port() {
     while [ ! -s "$ready_file" ] && [ "$counter" -lt 20 ]; do
-        echo -e "waiting for ready file..."
+        echo "waiting for ready file..."
         sleep 0.1
         counter=$((counter+ 1))
     done
 
-    if test -e $ready_file; then
-        echo -e "found ready file, starting client..."
+    if test -e "$ready_file"; then
+        echo "found ready file, starting client..."
 
         # get created port 0 ephemeral port
-        port=`cat $ready_file`
+        port=$(cat "$ready_file")
     else
-        echo -e "NO ready file ending test..."
+        echo "NO ready file ending test..."
         do_cleanup
         exit 1
     fi
 }
 
 remove_ready_file() {
-    if test -e $ready_file; then
-        echo -e "removing existing ready file"
-    rm $ready_file
+    if test -e "$ready_file"; then
+        echo "removing existing ready file"
+        rm "$ready_file"
     fi
 }
 
@@ -69,60 +71,66 @@ do_cleanup() {
 do_trap() {
     echo "got trap"
     do_cleanup
-    exit -1
+    exit 1
 }
 
 trap do_trap INT TERM
 
-[ ! -x ./examples/sftpclient/wolfsftp ] && echo -e "\n\nClient doesn't exist" && exit 1
+[ ! -x ./examples/sftpclient/wolfsftp ] && echo && echo "Client doesn't exist" && exit 1
 
 if [ $nonblockingOnly = 0 ]; then
     echo "Test basic connection"
-    ./examples/echoserver/echoserver -1 -R $ready_file &
+    ./examples/echoserver/echoserver -1 -R "$ready_file" &
     server_pid=$!
+    counter=0
     create_port
-    echo "exit" | ./examples/sftpclient/wolfsftp -u jill -P upthehill -p $port
+    echo "exit" | ./examples/sftpclient/wolfsftp -u jill -P upthehill -p "$port"
     RESULT=$?
     remove_ready_file
     if [ $RESULT -ne 0 ]; then
-        echo -e "\n\nfailed to connect"
+        echo
+        echo "failed to connect"
         do_cleanup
         exit 1
     fi
 fi
 
 # Test non blocking connection
 echo "Test non blocking connection"
-./examples/echoserver/echoserver -N -1 -R $ready_file &
+./examples/echoserver/echoserver -N -1 -R "$ready_file" &
 server_pid=$!
+counter=0
 create_port
-echo "exit" | ./examples/sftpclient/wolfsftp -N -u jill -P upthehill -p $port
+echo "exit" | ./examples/sftpclient/wolfsftp -N -u jill -P upthehill -p "$port"
 RESULT=$?
 remove_ready_file
 if [ $RESULT -ne 0 ]; then
-    echo -e "\n\nfailed to connect"
+    echo
+    echo "failed to connect"
     do_cleanup
     exit 1
 fi
 
 # Test of setting directory
 if [ $nonblockingOnly = 0 ]; then
     echo "Test of setting directory"
-    PWD=`pwd`
-    ./examples/echoserver/echoserver -d $PWD/examples -1 -R $ready_file &
+    ./examples/echoserver/echoserver -d "$PWD"/examples -1 -R "$ready_file" &
     server_pid=$!
+    counter=0
     create_port
-    echo "exit" | ./examples/sftpclient/wolfsftp -N -u jill -P upthehill -p $port
+    echo "exit" | ./examples/sftpclient/wolfsftp -N -u jill -P upthehill -p "$port"
     RESULT=$?
     remove_ready_file
     if [ $RESULT -ne 0 ]; then
-        echo -e "\n\nfailed to connect"
+        echo
+        echo "failed to connect"
         do_cleanup
         exit 1
     fi
 fi
 
-echo -e "\nALL Tests Passed"
+echo
+echo "ALL Tests Passed"
 
 exit 0
 
diff --git a/src/ssh.c b/src/ssh.c
@@ -424,16 +424,15 @@ int wolfSSH_accept(WOLFSSH* ssh)
 
     /* check if data pending to be sent */
     if (ssh->outputBuffer.length > 0 &&
-            ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED) {
+            ssh->acceptState < ACCEPT_DONE) {
         if ((ssh->error = wolfSSH_SendPacket(ssh)) == WS_SUCCESS) {
             WLOG(WS_LOG_DEBUG, "Sent pending packet");
 
             /* adjust state, a couple of them use multiple sends */
             if (ssh->acceptState != ACCEPT_SERVER_VERSION_SENT &&
                 ssh->acceptState != ACCEPT_SERVER_USERAUTH_ACCEPT_SENT &&
                 ssh->acceptState != ACCEPT_SERVER_KEXINIT_SENT &&
-                ssh->acceptState != ACCEPT_KEYED &&
-                ssh->acceptState != ACCEPT_SERVER_CHANNEL_ACCEPT_SENT) {
+                ssh->acceptState != ACCEPT_KEYED) {
                 WLOG(WS_LOG_DEBUG, "Advancing accept state");
                 ssh->acceptState++;
             }

diff --git a/src/wolfsftp.c b/src/wolfsftp.c
@@ -1130,22 +1130,6 @@ int wolfSSH_SFTP_accept(WOLFSSH* ssh)
     if (ssh->error == WS_WANT_READ || ssh->error == WS_WANT_WRITE)
         ssh->error = WS_SUCCESS;
 
-    /* check accept is done, if not call wolfSSH accept */
-    if (ssh->acceptState < ACCEPT_CLIENT_SESSION_ESTABLISHED) {
-        byte name[] = "sftp";
-
-        WLOG(WS_LOG_SFTP, "Trying to do SSH accept first");
-        if ((ret = wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM,
-                            name, sizeof(name) - 1)) != WS_SUCCESS) {
-            WLOG(WS_LOG_SFTP, "Unable to set subsystem channel type");
-            return ret;
-        }
-
-        if ((ret = wolfSSH_accept(ssh)) != WS_SUCCESS) {
-            return ret;
-        }
-    }
-
     switch (ssh->sftpState) {
         case SFTP_BEGIN:
         case SFTP_EXT:

diff --git a/wolfssh/internal.h b/wolfssh/internal.h
@@ -1035,17 +1035,7 @@ enum AcceptStates {
     ACCEPT_SERVER_USERAUTH_ACCEPT_SENT,
     ACCEPT_CLIENT_USERAUTH_DONE,
     ACCEPT_SERVER_USERAUTH_SENT,
-    ACCEPT_SERVER_CHANNEL_ACCEPT_SENT,
-    ACCEPT_CLIENT_SESSION_ESTABLISHED,
-#ifdef WOLFSSH_SCP
-    ACCEPT_INIT_SCP_TRANSFER,
-#endif
-#ifdef WOLFSSH_SFTP
-    ACCEPT_INIT_SFTP,
-#endif
-#ifdef WOLFSSH_AGENT
-    ACCEPT_INIT_AGENT,
-#endif
+    ACCEPT_DONE
 };