From 567b68129943a1cceab1d7b4c68e2a4ba011cdc0 Mon Sep 17 00:00:00 2001 From: Mikhail Chalov Date: Tue, 15 Nov 2022 14:39:30 -0800 Subject: Minimize unsafe C functions usage - replace strcat() and strcpy() (and strncat() and strncpy()) with custom safe_strcat() and safe_strcpy() functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MariaDB code base uses strcat() and strcpy() in several places. These are known to have memory safety issues and their usage is discouraged. Common security scanners like Flawfinder flags them. In MariaDB we should start using modern and safer variants on these functions. This is similar to memory issues fixes in 19af1890b56c6c147c296479bb6a4ad00fa59dbb and 9de9f105b5cb88249acc39af73d32af337d6fd5f but now replace use of strcat() and strcpy() with safer options strncat() and strncpy(). However, add '\0' forcefully to make sure the result string is correct since for these two functions it is not guaranteed what new string will be null-terminated. Example: size_t dest_len = sizeof(g->Message); strncpy(g->Message, "Null json tree", dest_len); strncat(g->Message, ":", sizeof(g->Message) - strlen(g->Message)); size_t wrote_sz = strlen(g->Message); size_t cur_len = wrote_sz >= dest_len ? dest_len - 1 : wrote_sz; g->Message[cur_len] = '\0'; All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services -- Reviewer and co-author Vicențiu Ciorbaru -- Reviewer additions: * The initial function implementation was flawed. Replaced with a simpler and also correct version. * Simplified code by making use of snprintf instead of chaining strcat. * Simplified code by removing dynamic string construction in the first place and using static strings if possible. See connect storage engine changes. --- storage/connect/javaconn.cpp | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) (limited to 'storage/connect/javaconn.cpp') diff --git a/storage/connect/javaconn.cpp b/storage/connect/javaconn.cpp index 0dc467aa7ee..3d1dfbc3f26 100644 --- a/storage/connect/javaconn.cpp +++ b/storage/connect/javaconn.cpp @@ -33,6 +33,8 @@ #define NODW #endif // !_WIN32 +#include + /***********************************************************************/ /* Required objects includes. */ /***********************************************************************/ @@ -231,15 +233,16 @@ bool JAVAConn::GetJVM(PGLOBAL g) #if defined(_WIN32) for (ntry = 0; !LibJvm && ntry < 3; ntry++) { if (!ntry && JvmPath) { - strcat(strcpy(soname, JvmPath), "\\jvm.dll"); + snprintf(soname, sizeof(soname), "%s\\jvm.dll", JvmPath); + ntry = 3; // No other try } else if (ntry < 2 && getenv("JAVA_HOME")) { - strcpy(soname, getenv("JAVA_HOME")); + safe_strcpy(soname, sizeof(soname), getenv("JAVA_HOME")); if (ntry == 1) - strcat(soname, "\\jre"); + safe_strcat(soname, sizeof(soname), "\\jre"); - strcat(soname, "\\bin\\client\\jvm.dll"); + safe_strcat(soname, sizeof(soname), "\\bin\\client\\jvm.dll"); } else { // Try to find it through the registry char version[16]; @@ -247,11 +250,12 @@ bool JAVAConn::GetJVM(PGLOBAL g) LONG rc; DWORD BufferSize = 16; - strcpy(soname, "jvm.dll"); // In case it fails + safe_strcpy(soname, sizeof(soname), "jvm.dll"); // In case it fails if ((rc = RegGetValue(HKEY_LOCAL_MACHINE, javaKey, "CurrentVersion", RRF_RT_ANY, NULL, (PVOID)&version, &BufferSize)) == ERROR_SUCCESS) { - strcat(strcat(javaKey, "\\"), version); + safe_strcat(javaKey, sizeof(javaKey), "\\"); + safe_strcat(javaKey, sizeof(javaKey), version); BufferSize = sizeof(soname); if ((rc = RegGetValue(HKEY_LOCAL_MACHINE, javaKey, "RuntimeLib", @@ -272,11 +276,11 @@ bool JAVAConn::GetJVM(PGLOBAL g) char buf[256]; DWORD rc = GetLastError(); - snprintf(g->Message, sizeof(g->Message), MSG(DLL_LOAD_ERROR), rc, soname); FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, NULL, rc, 0, (LPTSTR)buf, sizeof(buf), NULL); - strcat(strcat(g->Message, ": "), buf); + snprintf(g->Message, sizeof(g->Message), MSG(DLL_LOAD_ERROR)": %s", rc, + soname, buf); } else if (!(CreateJavaVM = (CRTJVM)GetProcAddress((HINSTANCE)LibJvm, "JNI_CreateJavaVM"))) { snprintf(g->Message, sizeof(g->Message), MSG(PROCADD_ERROR), GetLastError(), "JNI_CreateJavaVM"); @@ -301,13 +305,14 @@ bool JAVAConn::GetJVM(PGLOBAL g) for (ntry = 0; !LibJvm && ntry < 2; ntry++) { if (!ntry && JvmPath) { - strcat(strcpy(soname, JvmPath), "/libjvm.so"); + snprintf(soname, sizeof(soname), "%s/libjvm.so", JvmPath); ntry = 2; } else if (!ntry && getenv("JAVA_HOME")) { // TODO: Replace i386 by a better guess - strcat(strcpy(soname, getenv("JAVA_HOME")), "/jre/lib/i386/client/libjvm.so"); + snprintf(soname, sizeof(soname), "%s/jre/lib/i386/client/libjvm.so", + getenv("JAVA_HOME")); } else { // Will need LD_LIBRARY_PATH to be set - strcpy(soname, "libjvm.so"); + safe_strcpy(soname, sizeof(soname), "libjvm.so"); ntry = 2; } // endelse -- cgit v1.2.1