From bb6d2b1b7453139bc252bedbde0906c6ed10e25d Mon Sep 17 00:00:00 2001 From: Andrew Gaul Date: Sat, 8 May 2021 02:48:47 +0900 Subject: [PATCH] Replace snprintf with string and ostringstream (#1649) These uses are probably safe from a buffer overflow perspective but can cause data race issues in logging due to static buffers. --- src/curl.cpp | 6 ++---- src/fdcache.cpp | 2 +- src/s3fs_logger.cpp | 23 ++++++++++++++++++++++- src/s3fs_logger.h | 38 ++++++-------------------------------- src/string_util.cpp | 5 ++--- 5 files changed, 33 insertions(+), 41 deletions(-) diff --git a/src/curl.cpp b/src/curl.cpp index 8de4428..43829a8 100644 --- a/src/curl.cpp +++ b/src/curl.cpp @@ -2784,11 +2784,9 @@ int S3fsCurl::GetIAMv2ApiToken() responseHeaders.clear(); bodydata.Clear(); - // maximum allowed value is 21600, so 6 bytes for the C string - char ttlstr[6]; - snprintf(ttlstr, sizeof(ttlstr), "%d", S3fsCurl::IAMv2_token_ttl); + std::string ttlstr = str(S3fsCurl::IAMv2_token_ttl); requestHeaders = curl_slist_sort_insert(requestHeaders, S3fsCurl::IAMv2_token_ttl_hdr.c_str(), - ttlstr); + ttlstr.c_str()); curl_easy_setopt(hCurl, CURLOPT_PUT, true); curl_easy_setopt(hCurl, CURLOPT_URL, url.c_str()); curl_easy_setopt(hCurl, CURLOPT_WRITEDATA, (void*)&bodydata); diff --git a/src/fdcache.cpp b/src/fdcache.cpp index 29ed467..5d106cc 100644 --- a/src/fdcache.cpp +++ b/src/fdcache.cpp @@ -909,7 +909,7 @@ bool FdManager::CheckAllCache() } // print head message - S3FS_PRN_CACHE(fp, CACHEDBG_FMT_HEAD, S3fsLog::GetCurrentTime()); + S3FS_PRN_CACHE(fp, CACHEDBG_FMT_HEAD, S3fsLog::GetCurrentTime().c_str()); // Loop in directory of cache file's stats std::string top_path = CacheFileStat::GetCacheFileStatTopDir(); diff --git a/src/s3fs_logger.cpp b/src/s3fs_logger.cpp index 16fb8d9..ab77b0a 100644 --- a/src/s3fs_logger.cpp +++ b/src/s3fs_logger.cpp @@ -19,6 +19,8 @@ */ #include +#include +#include #include #include "common.h" @@ -35,7 +37,6 @@ S3fsLog* S3fsLog::pSingleton = NULL; S3fsLog::s3fs_log_level S3fsLog::debug_level = S3fsLog::LEVEL_CRIT; FILE* S3fsLog::logfp = NULL; std::string* S3fsLog::plogfile = NULL; -char S3fsLog::current_time[64] = ""; bool S3fsLog::time_stamp = true; //------------------------------------------------------------------- @@ -46,6 +47,26 @@ bool S3fsLog::IsS3fsLogLevel(s3fs_log_level level) return (level == (S3fsLog::debug_level & level)); } +std::string S3fsLog::GetCurrentTime() +{ + std::ostringstream current_time; + if(time_stamp){ + struct timeval now; + struct timespec tsnow; + struct tm res; + char tmp[32]; + if(-1 == clock_gettime(S3FS_CLOCK_MONOTONIC, &tsnow)){ + now.tv_sec = tsnow.tv_sec; + now.tv_usec = (tsnow.tv_nsec / 1000); + }else{ + gettimeofday(&now, NULL); + } + strftime(tmp, sizeof(tmp), "%Y-%m-%dT%H:%M:%S", gmtime_r(&now.tv_sec, &res)); + current_time << tmp << "." << std::setfill('0') << std::setw(3) << (now.tv_usec / 1000) << "Z "; + } + return current_time.str(); +} + bool S3fsLog::SetLogfile(const char* pfile) { if(!S3fsLog::pSingleton){ diff --git a/src/s3fs_logger.h b/src/s3fs_logger.h index e795242..43cdc0e 100644 --- a/src/s3fs_logger.h +++ b/src/s3fs_logger.h @@ -32,12 +32,6 @@ #define S3FS_CLOCK_MONOTONIC CLOCK_MONOTONIC #endif -#if defined(__APPLE__) -#define S3FSLOG_TIME_FMT "%s.%03dZ " -#else -#define S3FSLOG_TIME_FMT "%s.%03ldZ " -#endif - //------------------------------------------------------------------- // S3fsLog class //------------------------------------------------------------------- @@ -62,7 +56,6 @@ class S3fsLog static s3fs_log_level debug_level; static FILE* logfp; static std::string* plogfile; - static char current_time[64]; static bool time_stamp; protected: @@ -87,26 +80,7 @@ class S3fsLog LEVEL_ERR == (level & LEVEL_DBG) ? LOG_ERR : LOG_CRIT ); } - static const char* GetCurrentTime() - { - if(time_stamp){ - struct timeval now; - struct timespec tsnow; - struct tm res; - char tmp[32]; - if(-1 == clock_gettime(S3FS_CLOCK_MONOTONIC, &tsnow)){ - now.tv_sec = tsnow.tv_sec; - now.tv_usec = (tsnow.tv_nsec / 1000); - }else{ - gettimeofday(&now, NULL); - } - strftime(tmp, sizeof(tmp), "%Y-%m-%dT%H:%M:%S", gmtime_r(&now.tv_sec, &res)); - snprintf(current_time, sizeof(current_time), S3FSLOG_TIME_FMT, tmp, (now.tv_usec / 1000)); - }else{ - current_time[0] = '\0'; - } - return current_time; - } + static std::string GetCurrentTime(); static const char* GetLevelString(s3fs_log_level level) { @@ -172,7 +146,7 @@ class S3fsLog if(S3fsLog::IsS3fsLogLevel(level)){ \ if(foreground || S3fsLog::IsSetLogFile()){ \ S3fsLog::SeekEnd(); \ - fprintf(S3fsLog::GetOutputLogFile(), "%s%s%s:%s(%d): " fmt "%s\n", S3fsLog::GetCurrentTime(), S3fsLog::GetLevelString(level), __FILE__, __func__, __LINE__, __VA_ARGS__); \ + fprintf(S3fsLog::GetOutputLogFile(), "%s%s%s:%s(%d): " fmt "%s\n", S3fsLog::GetCurrentTime().c_str(), S3fsLog::GetLevelString(level), __FILE__, __func__, __LINE__, __VA_ARGS__); \ S3fsLog::Flush(); \ }else{ \ syslog(S3fsLog::GetSyslogLevel(level), "%s%s:%s(%d): " fmt "%s", instance_name.c_str(), __FILE__, __func__, __LINE__, __VA_ARGS__); \ @@ -185,7 +159,7 @@ class S3fsLog if(S3fsLog::IsS3fsLogLevel(level)){ \ if(foreground || S3fsLog::IsSetLogFile()){ \ S3fsLog::SeekEnd(); \ - fprintf(S3fsLog::GetOutputLogFile(), "%s%s%s%s:%s(%d): " fmt "%s\n", S3fsLog::GetCurrentTime(), S3fsLog::GetLevelString(level), S3fsLog::GetS3fsLogNest(nest), __FILE__, __func__, __LINE__, __VA_ARGS__); \ + fprintf(S3fsLog::GetOutputLogFile(), "%s%s%s%s:%s(%d): " fmt "%s\n", S3fsLog::GetCurrentTime().c_str(), S3fsLog::GetLevelString(level), S3fsLog::GetS3fsLogNest(nest), __FILE__, __func__, __LINE__, __VA_ARGS__); \ S3fsLog::Flush(); \ }else{ \ syslog(S3fsLog::GetSyslogLevel(level), "%s%s" fmt "%s", instance_name.c_str(), S3fsLog::GetS3fsLogNest(nest), __VA_ARGS__); \ @@ -197,7 +171,7 @@ class S3fsLog do{ \ if(foreground || S3fsLog::IsSetLogFile()){ \ S3fsLog::SeekEnd(); \ - fprintf(S3fsLog::GetOutputLogFile(), "%s[CURL DBG] " fmt "%s\n", S3fsLog::GetCurrentTime(), __VA_ARGS__); \ + fprintf(S3fsLog::GetOutputLogFile(), "%s[CURL DBG] " fmt "%s\n", S3fsLog::GetCurrentTime().c_str(), __VA_ARGS__); \ S3fsLog::Flush(); \ }else{ \ syslog(S3fsLog::GetSyslogLevel(S3fsLog::LEVEL_CRIT), "%s" fmt "%s", instance_name.c_str(), __VA_ARGS__); \ @@ -221,7 +195,7 @@ class S3fsLog do{ \ if(foreground || S3fsLog::IsSetLogFile()){ \ S3fsLog::SeekEnd(); \ - fprintf(S3fsLog::GetOutputLogFile(), "%s%s%s%s:%s(%d): " fmt "%s\n", S3fsLog::GetCurrentTime(), S3fsLog::GetLevelString(S3fsLog::LEVEL_INFO), S3fsLog::GetS3fsLogNest(0), __FILE__, __func__, __LINE__, __VA_ARGS__, ""); \ + fprintf(S3fsLog::GetOutputLogFile(), "%s%s%s%s:%s(%d): " fmt "%s\n", S3fsLog::GetCurrentTime().c_str(), S3fsLog::GetLevelString(S3fsLog::LEVEL_INFO), S3fsLog::GetS3fsLogNest(0), __FILE__, __func__, __LINE__, __VA_ARGS__, ""); \ S3fsLog::Flush(); \ }else{ \ syslog(S3fsLog::GetSyslogLevel(S3fsLog::LEVEL_INFO), "%s%s" fmt "%s", instance_name.c_str(), S3fsLog::GetS3fsLogNest(0), __VA_ARGS__, ""); \ @@ -232,7 +206,7 @@ class S3fsLog do{ \ if(foreground || S3fsLog::IsSetLogFile()){ \ S3fsLog::SeekEnd(); \ - fprintf(S3fsLog::GetOutputLogFile(), "%s%s" fmt "%s\n", S3fsLog::GetCurrentTime(), S3fsLog::GetLevelString(S3fsLog::LEVEL_INFO), __VA_ARGS__, ""); \ + fprintf(S3fsLog::GetOutputLogFile(), "%s%s" fmt "%s\n", S3fsLog::GetCurrentTime().c_str(), S3fsLog::GetLevelString(S3fsLog::LEVEL_INFO), __VA_ARGS__, ""); \ S3fsLog::Flush(); \ }else{ \ syslog(S3fsLog::GetSyslogLevel(S3fsLog::LEVEL_INFO), "%s" fmt "%s", instance_name.c_str(), __VA_ARGS__, ""); \ diff --git a/src/string_util.cpp b/src/string_util.cpp index 862641c..43f0737 100644 --- a/src/string_util.cpp +++ b/src/string_util.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -59,9 +60,7 @@ template<> std::string str(struct timespec value) { std::ostringstream s; s << value.tv_sec; if(value.tv_nsec != 0){ - char buf[16]; - snprintf(buf, sizeof(buf), "%09lld", static_cast(value.tv_nsec)); - s << "." << buf; + s << "." << std::setfill('0') << std::setw(9) << value.tv_nsec; } return s.str(); }