fixes for defects identified by cppcheck and clang-tidy on --enable-d…

…ebug builds: null deref in tests/api.c:load_pem_key_file_as_der(), redundant declarations in wolfcrypt/benchmark/benchmark.c, and numerous unchecked XSNPRINTF()s in wolfcrypt/src/logging.c and src/internal.c.
gojimmypi · Aug 8, 2024 · 763ced6 · 763ced6
1 parent 5f6067c
commit 763ced6
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 28 deletions.
diff --git a/src/internal.c b/src/internal.c
@@ -20653,7 +20653,11 @@ static void LogAlert(int type)
     typeStr = AlertTypeToString(type);
     if (typeStr != NULL) {
         char buff[60];
-        XSNPRINTF(buff, sizeof(buff), "Alert type: %s", typeStr);
+        if (XSNPRINTF(buff, sizeof(buff), "Alert type: %s", typeStr)
+            >= (int)sizeof(buff))
+        {
+            buff[sizeof(buff) - 1] = 0;
+        }
         WOLFSSL_MSG(buff);
     }
 #else

diff --git a/tests/api.c b/tests/api.c
@@ -78089,9 +78089,11 @@ static int load_pem_key_file_as_der(const char* privKeyFile, DerBuffer** pDer,
     (void)encInfo; /* not used in this test */
 
 #ifdef DEBUG_WOLFSSL
-    fprintf(stderr, "%s (%d): Loading PEM %s (len %d) to DER (len %d)\n",
-        (ret == 0) ? "Success" : "Failure", ret, privKeyFile, (int)key_sz,
-        (*pDer)->length);
+    if (*pDer != NULL) {
+        fprintf(stderr, "%s (%d): Loading PEM %s (len %d) to DER (len %d)\n",
+                (ret == 0) ? "Success" : "Failure", ret, privKeyFile,
+                (int)key_sz, (*pDer)->length);
+    }
 #endif
 
     return ret;

diff --git a/wolfcrypt/benchmark/benchmark.c b/wolfcrypt/benchmark/benchmark.c
@@ -1663,18 +1663,6 @@ static const char* bench_result_words3[][5] = {
                                           const char *desc_extra);
 #endif
 
-#if defined(DEBUG_WOLFSSL) && !defined(HAVE_VALGRIND) && \
-        !defined(HAVE_STACK_SIZE)
-#ifdef __cplusplus
-    extern "C" {
-#endif
-    WOLFSSL_API int wolfSSL_Debugging_ON(void);
-    WOLFSSL_API void wolfSSL_Debugging_OFF(void);
-#ifdef __cplusplus
-    }  /* extern "C" */
-#endif
-#endif
-
 #if !defined(WC_NO_RNG) && \
         ((!defined(NO_RSA) && !defined(WOLFSSL_RSA_VERIFY_ONLY)) \
         || !defined(NO_DH) || defined(WOLFSSL_KEY_GEN) || defined(HAVE_ECC) \

diff --git a/wolfcrypt/src/logging.c b/wolfcrypt/src/logging.c
@@ -471,26 +471,48 @@ void WOLFSSL_BUFFER(const byte* buffer, word32 length)
 
     while (buflen > 0) {
         int bufidx = 0;
-        XSNPRINTF(&line[bufidx], sizeof(line)-bufidx, "\t");
+        if (XSNPRINTF(&line[bufidx], sizeof(line)-bufidx, "\t")
+            >= (int)sizeof(line) - bufidx)
+        {
+            return;
+        }
         bufidx++;
 
         for (i = 0; i < LINE_LEN; i++) {
             if (i < buflen) {
-                XSNPRINTF(&line[bufidx], sizeof(line)-bufidx, "%02x ", buffer[i]);
+                if (XSNPRINTF(&line[bufidx], sizeof(line)-bufidx, "%02x ",
+                              buffer[i]) >= (int)sizeof(line) - bufidx)
+                {
+                    return;
+                }
             }
             else {
-                XSNPRINTF(&line[bufidx], sizeof(line)-bufidx, "   ");
+                if (XSNPRINTF(&line[bufidx], sizeof(line)-bufidx, "   ")
+                    >= (int)sizeof(line) - bufidx)
+                {
+                    return;
+                }
             }
             bufidx += 3;
         }
 
-        XSNPRINTF(&line[bufidx], sizeof(line)-bufidx, "| ");
+        if (XSNPRINTF(&line[bufidx], sizeof(line)-bufidx, "| ")
+            >= (int)sizeof(line) - bufidx)
+        {
+            return;
+        }
         bufidx++;
 
         for (i = 0; i < LINE_LEN; i++) {
             if (i < buflen) {
-                XSNPRINTF(&line[bufidx], sizeof(line)-bufidx,
-                     "%c", 31 < buffer[i] && buffer[i] < 127 ? buffer[i] : '.');
+                if (XSNPRINTF(&line[bufidx], sizeof(line)-bufidx,
+                              "%c", 31 < buffer[i] && buffer[i] < 127
+                              ? buffer[i]
+                              : '.')
+                    >= (int)sizeof(line) - bufidx)
+                {
+                    return;
+                }
                 bufidx++;
             }
         }
@@ -506,7 +528,11 @@ void WOLFSSL_ENTER(const char* msg)
 {
     if (loggingEnabled) {
         char buffer[WOLFSSL_MAX_ERROR_SZ];
-        XSNPRINTF(buffer, sizeof(buffer), "wolfSSL Entering %s", msg);
+        if (XSNPRINTF(buffer, sizeof(buffer), "wolfSSL Entering %s", msg)
+            >= (int)sizeof(buffer))
+        {
+            buffer[sizeof(buffer) - 1] = 0;
+        }
         wolfssl_log(ENTER_LOG, NULL, 0, buffer);
     }
 }
@@ -516,7 +542,11 @@ void WOLFSSL_ENTER2(const char *file, int line, const char* msg)
 {
     if (loggingEnabled) {
         char buffer[WOLFSSL_MAX_ERROR_SZ];
-        XSNPRINTF(buffer, sizeof(buffer), "wolfSSL Entering %s", msg);
+        if (XSNPRINTF(buffer, sizeof(buffer), "wolfSSL Entering %s", msg)
+            >= (int)sizeof(buffer))
+        {
+            buffer[sizeof(buffer) - 1] = 0;
+        }
         wolfssl_log(ENTER_LOG, file, line, buffer);
     }
 }
@@ -527,8 +557,12 @@ void WOLFSSL_LEAVE(const char* msg, int ret)
 {
     if (loggingEnabled) {
         char buffer[WOLFSSL_MAX_ERROR_SZ];
-        XSNPRINTF(buffer, sizeof(buffer), "wolfSSL Leaving %s, return %d",
-                msg, ret);
+        if (XSNPRINTF(buffer, sizeof(buffer), "wolfSSL Leaving %s, return %d",
+                      msg, ret)
+            >= (int)sizeof(buffer))
+        {
+            buffer[sizeof(buffer) - 1] = 0;
+        }
         wolfssl_log(LEAVE_LOG, NULL, 0, buffer);
     }
 }
@@ -538,8 +572,12 @@ void WOLFSSL_LEAVE2(const char *file, int line, const char* msg, int ret)
 {
     if (loggingEnabled) {
         char buffer[WOLFSSL_MAX_ERROR_SZ];
-        XSNPRINTF(buffer, sizeof(buffer), "wolfSSL Leaving %s, return %d",
-                msg, ret);
+        if (XSNPRINTF(buffer, sizeof(buffer), "wolfSSL Leaving %s, return %d",
+                      msg, ret)
+            >= (int)sizeof(buffer))
+        {
+            buffer[sizeof(buffer) - 1] = 0;
+        }
         wolfssl_log(LEAVE_LOG, file, line, buffer);
     }
 }