From ef695e4627013e6deecf9cff0cc13f3afeaf8229 Mon Sep 17 00:00:00 2001 From: "Mark A. Tsuchida" Date: Wed, 24 Jan 2024 18:16:29 -0600 Subject: [PATCH] MMDevice: Remove need for _CRT_SECURE_NO_WARNINGS That is, remove use of strcpy(), strncpy(), and getenv(). Replace str[n]cpy() with snprintf(). Add tests in the case of CDeviceUtils::CopyLimitedString() (previous implementation had a bug where it didn't null-terminate if the source string was exactly MM::MaxStrLength - 1 chars long). The use of getenv() was dead code, so remove entirely. --- MMDevice/DeviceUtils.cpp | 37 ++++++------------------- MMDevice/DeviceUtils.h | 1 - MMDevice/MMDeviceConstants.h | 3 ++ MMDevice/ModuleInterface.cpp | 5 ++-- MMDevice/meson.build | 4 +-- MMDevice/unittest/DeviceUtils-Tests.cpp | 26 +++++++++++++++++ MMDevice/unittest/meson.build | 1 + 7 files changed, 41 insertions(+), 36 deletions(-) create mode 100644 MMDevice/unittest/DeviceUtils-Tests.cpp diff --git a/MMDevice/DeviceUtils.cpp b/MMDevice/DeviceUtils.cpp index edcec6967..5ef84315f 100644 --- a/MMDevice/DeviceUtils.cpp +++ b/MMDevice/DeviceUtils.cpp @@ -34,18 +34,16 @@ char CDeviceUtils::m_pszBuffer[MM::MaxStrLength]={""}; /** - * Copies strings with predefined size limit. + * Copies string up to MM::MaxStrLength - 1 characters, truncating if necessary + * and ensuring the result is null-terminated. + * + * Behavior is undefined unless dest points to a buffer with size at least + * MM::MaxStrLength and src points to a null-terminated string. */ -bool CDeviceUtils::CopyLimitedString(char* target, const char* source) +bool CDeviceUtils::CopyLimitedString(char* dest, const char* src) { - strncpy(target, source, MM::MaxStrLength - 1); - if ((MM::MaxStrLength - 1) < strlen(source)) - { - target[MM::MaxStrLength - 1] = 0; - return false; // string truncated - } - else - return true; + snprintf(dest, MM::MaxStrLength, "%s", src); + return strlen(src) <= MM::MaxStrLength; } /** @@ -169,22 +167,3 @@ void CDeviceUtils::NapMicros(unsigned long period) usleep(period); #endif } - - -bool CDeviceUtils::CheckEnvironment(std::string env) -{ - bool bvalue = false; - if( 0 < env.length()) - { - char *pvalue = ::getenv(env.c_str()); - if( 0 != pvalue) - { - if( 0 != *pvalue) - { - char initial = (char)tolower(*pvalue); - bvalue = ('0' != initial) && ('f' != initial) && ( 'n' != initial); - } - } - } - return bvalue; -} diff --git a/MMDevice/DeviceUtils.h b/MMDevice/DeviceUtils.h index 32eb6577c..cd8e705b6 100644 --- a/MMDevice/DeviceUtils.h +++ b/MMDevice/DeviceUtils.h @@ -37,7 +37,6 @@ class CDeviceUtils static void SleepMs(long ms); static void NapMicros(unsigned long microsecs); static std::string HexRep(std::vector ); - static bool CheckEnvironment(std::string environment); private: static char m_pszBuffer[MM::MaxStrLength]; }; diff --git a/MMDevice/MMDeviceConstants.h b/MMDevice/MMDeviceConstants.h index 23c8cbc18..956b05474 100644 --- a/MMDevice/MMDeviceConstants.h +++ b/MMDevice/MMDeviceConstants.h @@ -78,6 +78,9 @@ namespace MM { + // Maximum length copied into various char buffers. + // Code providing buffer should assume excludes null terminator. + // Code filling buffer should assume includes null terminator. const int MaxStrLength = 1024; // system-wide property names diff --git a/MMDevice/ModuleInterface.cpp b/MMDevice/ModuleInterface.cpp index 0a3a8a520..1454c3e2f 100644 --- a/MMDevice/ModuleInterface.cpp +++ b/MMDevice/ModuleInterface.cpp @@ -85,7 +85,7 @@ MODULE_API bool GetDeviceName(unsigned deviceIndex, char* name, unsigned bufLen) if (deviceName.size() >= bufLen) return false; // buffer too small, can't truncate the name - strcpy(name, deviceName.c_str()); + std::snprintf(name, bufLen, "%s", deviceName.c_str()); return true; } @@ -115,8 +115,7 @@ MODULE_API bool GetDeviceDescription(const char* deviceName, char* description, if (it == g_registeredDevices.end()) return false; - strncpy(description, it->description_.c_str(), bufLen - 1); - + std::snprintf(description, bufLen, "%s", it->description_.c_str()); return true; } diff --git a/MMDevice/meson.build b/MMDevice/meson.build index 094d22430..e29cb3522 100644 --- a/MMDevice/meson.build +++ b/MMDevice/meson.build @@ -48,9 +48,7 @@ mmdevice_lib = static_library( 'MMDevice', sources: mmdevice_sources, include_directories: mmdevice_include_dir, - cpp_args: build_mode_args + [ - '-D_CRT_SECURE_NO_WARNINGS', # TODO Eliminate the need - ], + cpp_args: build_mode_args, # MMDevice does not depend on any external library. This is a big advantage # in simplifing its usage (given hundreds of device adapters depending on # MMDevice), so think twice before adding dependencies. diff --git a/MMDevice/unittest/DeviceUtils-Tests.cpp b/MMDevice/unittest/DeviceUtils-Tests.cpp new file mode 100644 index 000000000..aab00a41c --- /dev/null +++ b/MMDevice/unittest/DeviceUtils-Tests.cpp @@ -0,0 +1,26 @@ +#include + +#include "DeviceUtils.h" + +#include +#include + +TEST_CASE("CopyLimitedString truncates", "[CopyLimitedString]") +{ + std::vector dest(MM::MaxStrLength, '.'); + const std::string src(MM::MaxStrLength + 10, '*'); + CHECK_FALSE(CDeviceUtils::CopyLimitedString(dest.data(), src.c_str())); + CHECK(dest[0] == '*'); + CHECK(dest[MM::MaxStrLength - 2] == '*'); + CHECK(dest[MM::MaxStrLength - 1] == '\0'); +} + +TEST_CASE("CopyLimitedString max untruncated len", "[CopyLimitedString]") +{ + std::vector dest(MM::MaxStrLength, '.'); + const std::string src(MM::MaxStrLength - 1, '*'); + CHECK(CDeviceUtils::CopyLimitedString(dest.data(), src.c_str())); + CHECK(dest[0] == '*'); + CHECK(dest[MM::MaxStrLength - 2] == '*'); + CHECK(dest[MM::MaxStrLength - 1] == '\0'); +} diff --git a/MMDevice/unittest/meson.build b/MMDevice/unittest/meson.build index dbca08cff..7692bc02a 100644 --- a/MMDevice/unittest/meson.build +++ b/MMDevice/unittest/meson.build @@ -10,6 +10,7 @@ catch2_with_main_dep = dependency( ) mmdevice_test_sources = files( + 'DeviceUtils-Tests.cpp', 'FloatPropertyTruncation-Tests.cpp', 'MMTime-Tests.cpp', )