From 23666e542afeabeaf21ae314a867b55c0155a4fc Mon Sep 17 00:00:00 2001 From: Alan Greer Date: Mon, 11 Sep 2023 18:13:55 +0100 Subject: [PATCH 1/2] Improve character array handling ind remove magic numbers from the PowerPMAC driver. --- pmacApp/powerPmacAsynPortSrc/sshDriver.cpp | 107 +++++++++++++++------ pmacApp/powerPmacAsynPortSrc/sshDriver.h | 11 ++- 2 files changed, 83 insertions(+), 35 deletions(-) diff --git a/pmacApp/powerPmacAsynPortSrc/sshDriver.cpp b/pmacApp/powerPmacAsynPortSrc/sshDriver.cpp index 6957a706..aaea3300 100755 --- a/pmacApp/powerPmacAsynPortSrc/sshDriver.cpp +++ b/pmacApp/powerPmacAsynPortSrc/sshDriver.cpp @@ -16,6 +16,14 @@ #include #include #include +#include + +#define SSH_PORT 22 +#define SSH_NOT_CONNECTED 0 +#define SSH_CONNECTED 1 +#define SSH_ERROR_TIMEOUT_MS 100 + +#define PPMAC_MAX_CHAR_LENGTH 8192 /* * Uncomment the DEBUG define and recompile for lots of @@ -95,10 +103,18 @@ SSHDriver::SSHDriver(const char *host) // Initialize internal SSH parameters auth_pw_ = 0; got_ = 0; - connected_ = 0; - // Username and password currently set to empty strings - strcpy(username_, ""); - strcpy(password_, ""); + connected_ = SSH_NOT_CONNECTED; + // Initialise username and password + username_ = NULL; + password_ = NULL; + + // Allocate memory required for the write method buffers + write_input_ = (char *)malloc(PPMAC_MAX_CHAR_LENGTH); + write_buffer_ = (char *)malloc(2*PPMAC_MAX_CHAR_LENGTH); + write_expected_echo_ = (char *)malloc(2*PPMAC_MAX_CHAR_LENGTH); + + // Allocate the memory required to store the host address + host_ = (char *)malloc(strlen(host)); // Store the host address strcpy(host_, host); @@ -121,6 +137,12 @@ SSHDriverStatus SSHDriver::setUsername(const char *username) static const char *functionName = "SSHDriver::setUsername"; debugPrint("%s : Method called\n", functionName); + // Allocate the memory for the username + if (username_){ + free(username_); + } + username_ = (char *)malloc(strlen(username)); + // Store the username strcpy(username_, username); @@ -140,6 +162,12 @@ SSHDriverStatus SSHDriver::setPassword(const char *password) static const char *functionName = "SSHDriver::setPassword"; debugPrint("%s : Method called\n", functionName); + // Allocate the memory for the password + if (password_){ + free(password_); + } + password_ = (char *)malloc(strlen(password)); + // Store the password strcpy(password_, password); @@ -190,7 +218,7 @@ SSHDriverStatus SSHDriver::connectSSH() sock_ = socket(AF_INET, SOCK_STREAM, 0); sin_.sin_family = AF_INET; - sin_.sin_port = htons(22); + sin_.sin_port = htons(SSH_PORT); sin_.sin_addr.s_addr = hostaddr; if (connect(sock_, (struct sockaddr*)(&sin_), sizeof(struct sockaddr_in)) != 0){ debugPrint("%s : socket failed to connect!\n", functionName); @@ -216,7 +244,7 @@ SSHDriverStatus SSHDriver::connectSSH() } // Here we now have a connection that will need to be closed - connected_ = 1; + connected_ = SSH_CONNECTED; // At this point the connection hasn't yet authenticated. The first thing to do // is check the hostkey's fingerprint against the known hosts. @@ -280,7 +308,7 @@ SSHDriverStatus SSHDriver::connectSSH() // Poll the underlying socket for bytes once connection established // Do not read or write using libssh2 until the socket has received // bytes from the server. These first bytes will contain the welcome - // message and then it is safe to proceed with reading and writing + // message and then it is safe to proceed with reading and writing // through the established connection. const char numfds = 1; struct pollfd pfds[numfds]; @@ -360,11 +388,10 @@ SSHDriverStatus SSHDriver::setBlocking(int blocking) */ SSHDriverStatus SSHDriver::flush() { - char buff[2048]; static const char *functionName = "SSHDriver::flush"; debugPrint("%s : Method called\n", functionName); - if (connected_ == 0){ + if (connected_ == SSH_NOT_CONNECTED){ debugPrint("%s : Not connected\n", functionName); return SSHDriverError; } @@ -376,7 +403,7 @@ SSHDriverStatus SSHDriver::flush() return SSHDriverError; } // Read out any remaining bytes from t he channel - rc = libssh2_channel_read(channel_, buff, 2048); + rc = libssh2_channel_read(channel_, write_buffer_, PPMAC_MAX_CHAR_LENGTH); if (rc > 0){ debugPrint("Flushed %d bytes\n", rc); } @@ -406,29 +433,33 @@ SSHDriverStatus SSHDriver::setErrorChecking(bool error_check) */ SSHDriverStatus SSHDriver::write(const char *buffer, size_t bufferSize, size_t *bytesWritten, int timeout) { - char input[2048]; static const char *functionName = "SSHDriver::write"; debugPrint("%s : Method called\n", functionName); *bytesWritten = 0; - if (connected_ == 0){ + if (connected_ == SSH_NOT_CONNECTED){ debugPrint("%s : Not connected\n", functionName); return SSHDriverError; } + if (bufferSize > PPMAC_MAX_CHAR_LENGTH){ + debugPrint("%s : write request exceeds the maximum buffer size\n", functionName); + return SSHDriverError; + } + timeval stime; timeval ctime; gettimeofday(&stime, NULL); long mtimeout = (stime.tv_usec / 1000) + timeout; long tnow = 0; - strncpy(input, buffer, bufferSize); - input[bufferSize] = 0; + strncpy(write_input_, buffer, bufferSize); + write_input_[bufferSize] = 0; flush(); LogComPrint("LogCom sshDriver Writing %02lu bytes => ", (unsigned long)bufferSize); LogComStrPrintEscapedNL(buffer, bufferSize); - int rc = libssh2_channel_write(channel_, input, bufferSize); + int rc = libssh2_channel_write(channel_, write_input_, bufferSize); if (rc > 0){ debugPrint("%s : %d bytes written\n", functionName, rc); *bytesWritten = rc; @@ -439,31 +470,31 @@ SSHDriverStatus SSHDriver::write(const char *buffer, size_t bufferSize, size_t * } // Now we need to read back the same numer of bytes, to remove the written string from the buffer + // Each of these buffers must be twice that of the input buffer to allow for the PowerPMAC responding + // with a \r\n for every \n that is written by the request int bytesToRead = *bytesWritten; int bytes = 0; - char buff[512]; rc = 0; int crCount = 0; // Count the number of \n characters sent // Build the expected ECHO string - char expected_response[512]; - memset(expected_response, 0, 512); + memset(write_expected_echo_, 0, 2*PPMAC_MAX_CHAR_LENGTH); int expected_index = 0; for (int index = 0; index < (int)*bytesWritten; index++){ if (buffer[index] == '\n'){ // CR, need to read back 1 extra character crCount++; - expected_response[expected_index] = '\r'; + write_expected_echo_[expected_index] = '\r'; expected_index++; } - expected_response[expected_index] = buffer[index]; + write_expected_echo_[expected_index] = buffer[index]; expected_index++; } bytesToRead += crCount; int matched = 0; while ((matched == 0) && (tnow < mtimeout)){ - rc = libssh2_channel_read(channel_, &buff[bytes], bytesToRead); + rc = libssh2_channel_read(channel_, &write_buffer_[bytes], bytesToRead); if (rc > 0){ bytes+=rc; bytesToRead-=rc; @@ -472,7 +503,7 @@ SSHDriverStatus SSHDriver::write(const char *buffer, size_t bufferSize, size_t * if (bytes >= expected_index){ matched = 1; for (int mid = 1; mid <= expected_index; mid++){ - if (buff[bytes-mid] != expected_response[expected_index-mid]){ + if (write_buffer_[bytes-mid] != write_expected_echo_[expected_index-mid]){ matched = 0; } } @@ -486,32 +517,32 @@ SSHDriverStatus SSHDriver::write(const char *buffer, size_t bufferSize, size_t * } if (error_checking_){ - if (buff[0] == '\r' && input[0] != '\r' && expected_index > 2){ + if (write_buffer_[0] == '\r' && write_input_[0] != '\r' && expected_index > 2){ caught_errors_++; debugPrint("Caught communication error\n"); debugPrint("Matched status: %d\n", matched); debugPrint("Input string: "); for (int index=0; index <= expected_index; index++){ - debugPrint("[%d] ", input[index]); + debugPrint("[%d] ", write_input_[index]); } debugPrint("\n"); debugPrint("Expected response: "); for (int index=0; index <= expected_index; index++){ - debugPrint("[%d] ", expected_response[index]); + debugPrint("[%d] ", write_expected_echo_[index]); } debugPrint("\n"); debugPrint("Actual response: "); for (int index=0; index <= expected_index; index++){ - debugPrint("[%d] ", buff[index]); + debugPrint("[%d] ", write_buffer_[index]); } debugPrint("\n"); } } - buff[bytes] = '\0'; + write_buffer_[bytes] = '\0'; LogComPrint("LogCom sshDriver Echoed %02d bytes => ", bytes); - LogComStrPrintEscapedNL(buff, bytes); + LogComStrPrintEscapedNL(write_buffer_, bytes); gettimeofday(&ctime, NULL); @@ -546,7 +577,7 @@ SSHDriverStatus SSHDriver::read(char *buffer, size_t bufferSize, size_t *bytesRe debugPrint("%s : Read terminator %d ", functionName, readTerm); debugStrPrintEscapedNL(&ch, sizeof(ch)); - if (connected_ == 0){ + if (connected_ == SSH_NOT_CONNECTED){ debugPrint("%s : Not connected\n", functionName); return SSHDriverError; } @@ -602,7 +633,7 @@ SSHDriverStatus SSHDriver::read(char *buffer, size_t bufferSize, size_t *bytesRe } if (error_checking_){ - if ((mtimeout - tnow) < 100){ + if ((mtimeout - tnow) < SSH_ERROR_TIMEOUT_MS){ debugPrint("Delay in read response: %ld ms\n", (timeout - (mtimeout - tnow))); debugPrint("Input buffer: %s\n", buffer); caught_delays_++; @@ -681,8 +712,8 @@ SSHDriverStatus SSHDriver::disconnectSSH() static const char *functionName = "SSHDriver::disconnect"; debugPrint("%s : Method called\n", functionName); - if (connected_ == 1){ - connected_ = 0; + if (connected_ == SSH_CONNECTED){ + connected_ = SSH_NOT_CONNECTED; libssh2_session_disconnect(session_, "Normal Shutdown"); libssh2_session_free(session_); @@ -704,6 +735,18 @@ SSHDriver::~SSHDriver() { static const char *functionName = "SSHDriver::~SSHDriver"; debugPrint("%s : Method called\n", functionName); + + free(write_input_); + free(write_buffer_); + free(write_expected_echo_); + + free(host_); + if (username_){ + free(username_); + } + if (password_){ + free(password_); + } } SSHDriverStatus SSHDriver::report(FILE *fp) diff --git a/pmacApp/powerPmacAsynPortSrc/sshDriver.h b/pmacApp/powerPmacAsynPortSrc/sshDriver.h index 5a87ea36..bc937ab2 100644 --- a/pmacApp/powerPmacAsynPortSrc/sshDriver.h +++ b/pmacApp/powerPmacAsynPortSrc/sshDriver.h @@ -69,11 +69,16 @@ class SSHDriver { struct sockaddr_in sin_; LIBSSH2_SESSION *session_; LIBSSH2_CHANNEL *channel_; - char host_[256]; - char username_[256]; - char password_[256]; + char *host_; + char *username_; + char *password_; off_t got_; + // Memory blocks used when writing to the PowerPMAC + char *write_input_; + char *write_buffer_; + char *write_expected_echo_; + SSHDriverStatus setBlocking(int blocking); bool error_checking_; From 898b70a97777342df8a5b9e80bfb9b0df0a21afe Mon Sep 17 00:00:00 2001 From: Alan Greer Date: Mon, 16 Oct 2023 17:20:02 +0100 Subject: [PATCH 2/2] Updated buffer name to read_buffer_. Updated MAX_CHAR_LENGTH to 7168. Added DOUBLE_MAX_LENGTH definition. --- pmacApp/powerPmacAsynPortSrc/sshDriver.cpp | 25 +++++++++++----------- pmacApp/powerPmacAsynPortSrc/sshDriver.h | 2 +- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/pmacApp/powerPmacAsynPortSrc/sshDriver.cpp b/pmacApp/powerPmacAsynPortSrc/sshDriver.cpp index aaea3300..1707e2b6 100755 --- a/pmacApp/powerPmacAsynPortSrc/sshDriver.cpp +++ b/pmacApp/powerPmacAsynPortSrc/sshDriver.cpp @@ -23,7 +23,8 @@ #define SSH_CONNECTED 1 #define SSH_ERROR_TIMEOUT_MS 100 -#define PPMAC_MAX_CHAR_LENGTH 8192 +#define PPMAC_MAX_CHAR_LENGTH 7168 +#define PPMAC_DOUBLE_MAX_LENGTH 2*PPMAC_MAX_CHAR_LENGTH /* * Uncomment the DEBUG define and recompile for lots of @@ -110,8 +111,8 @@ SSHDriver::SSHDriver(const char *host) // Allocate memory required for the write method buffers write_input_ = (char *)malloc(PPMAC_MAX_CHAR_LENGTH); - write_buffer_ = (char *)malloc(2*PPMAC_MAX_CHAR_LENGTH); - write_expected_echo_ = (char *)malloc(2*PPMAC_MAX_CHAR_LENGTH); + read_buffer_ = (char *)malloc(PPMAC_DOUBLE_MAX_LENGTH); + write_expected_echo_ = (char *)malloc(PPMAC_DOUBLE_MAX_LENGTH); // Allocate the memory required to store the host address host_ = (char *)malloc(strlen(host)); @@ -403,7 +404,7 @@ SSHDriverStatus SSHDriver::flush() return SSHDriverError; } // Read out any remaining bytes from t he channel - rc = libssh2_channel_read(channel_, write_buffer_, PPMAC_MAX_CHAR_LENGTH); + rc = libssh2_channel_read(channel_, read_buffer_, PPMAC_DOUBLE_MAX_LENGTH); if (rc > 0){ debugPrint("Flushed %d bytes\n", rc); } @@ -479,7 +480,7 @@ SSHDriverStatus SSHDriver::write(const char *buffer, size_t bufferSize, size_t * // Count the number of \n characters sent // Build the expected ECHO string - memset(write_expected_echo_, 0, 2*PPMAC_MAX_CHAR_LENGTH); + memset(write_expected_echo_, 0, PPMAC_DOUBLE_MAX_LENGTH); int expected_index = 0; for (int index = 0; index < (int)*bytesWritten; index++){ if (buffer[index] == '\n'){ @@ -494,7 +495,7 @@ SSHDriverStatus SSHDriver::write(const char *buffer, size_t bufferSize, size_t * bytesToRead += crCount; int matched = 0; while ((matched == 0) && (tnow < mtimeout)){ - rc = libssh2_channel_read(channel_, &write_buffer_[bytes], bytesToRead); + rc = libssh2_channel_read(channel_, &read_buffer_[bytes], bytesToRead); if (rc > 0){ bytes+=rc; bytesToRead-=rc; @@ -503,7 +504,7 @@ SSHDriverStatus SSHDriver::write(const char *buffer, size_t bufferSize, size_t * if (bytes >= expected_index){ matched = 1; for (int mid = 1; mid <= expected_index; mid++){ - if (write_buffer_[bytes-mid] != write_expected_echo_[expected_index-mid]){ + if (read_buffer_[bytes-mid] != write_expected_echo_[expected_index-mid]){ matched = 0; } } @@ -517,7 +518,7 @@ SSHDriverStatus SSHDriver::write(const char *buffer, size_t bufferSize, size_t * } if (error_checking_){ - if (write_buffer_[0] == '\r' && write_input_[0] != '\r' && expected_index > 2){ + if (read_buffer_[0] == '\r' && write_input_[0] != '\r' && expected_index > 2){ caught_errors_++; debugPrint("Caught communication error\n"); debugPrint("Matched status: %d\n", matched); @@ -533,16 +534,16 @@ SSHDriverStatus SSHDriver::write(const char *buffer, size_t bufferSize, size_t * debugPrint("\n"); debugPrint("Actual response: "); for (int index=0; index <= expected_index; index++){ - debugPrint("[%d] ", write_buffer_[index]); + debugPrint("[%d] ", read_buffer_[index]); } debugPrint("\n"); } } - write_buffer_[bytes] = '\0'; + read_buffer_[bytes] = '\0'; LogComPrint("LogCom sshDriver Echoed %02d bytes => ", bytes); - LogComStrPrintEscapedNL(write_buffer_, bytes); + LogComStrPrintEscapedNL(read_buffer_, bytes); gettimeofday(&ctime, NULL); @@ -737,7 +738,7 @@ SSHDriver::~SSHDriver() debugPrint("%s : Method called\n", functionName); free(write_input_); - free(write_buffer_); + free(read_buffer_); free(write_expected_echo_); free(host_); diff --git a/pmacApp/powerPmacAsynPortSrc/sshDriver.h b/pmacApp/powerPmacAsynPortSrc/sshDriver.h index bc937ab2..9f06472d 100644 --- a/pmacApp/powerPmacAsynPortSrc/sshDriver.h +++ b/pmacApp/powerPmacAsynPortSrc/sshDriver.h @@ -76,7 +76,7 @@ class SSHDriver { // Memory blocks used when writing to the PowerPMAC char *write_input_; - char *write_buffer_; + char *read_buffer_; char *write_expected_echo_; SSHDriverStatus setBlocking(int blocking);