From cb63b671279a9153e8912c795d1077071d989a6c Mon Sep 17 00:00:00 2001 From: Massimo Date: Sat, 1 Jun 2013 08:00:22 +0200 Subject: [PATCH 1/4] std::string for program name and param list Considering we have an std::vector here, I guess there's no problem in having std::strings as well. Note both leaks were properly noted... --- src/gflags.cc | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/src/gflags.cc b/src/gflags.cc index 01acd09..d4e6a4d 100644 --- a/src/gflags.cc +++ b/src/gflags.cc @@ -1463,11 +1463,16 @@ void GetAllFlags(vector* OUTPUT) { // These values are not protected by a Mutex because they are normally // set only once during program startup. -static const char* argv0 = "UNKNOWN"; // just the program name -static const char* cmdline = ""; // the entire command-line +// +// For some reason those were raw const char* allocated in SetArgv and nobody +// cared about releasing them. Oddly enough, SetArgv itself populates argvs +// which is a vector of std::string so it appears there's no real problem in +// using objects here. +static string argv0("UNKNOWN"); // just the program name +static string cmdline(""); // the entire command-line static vector argvs; -static uint32 argv_sum = 0; -static const char* program_usage = NULL; +static uint32 argv_sum = 0; // ?what is this for? +static string program_usage; void SetArgv(int argc, const char** argv) { static bool called_set_argv = false; @@ -1477,8 +1482,7 @@ void SetArgv(int argc, const char** argv) { called_set_argv = true; assert(argc > 0); // every program has at least a progname - argv0 = strdup(argv[0]); // small memory leak, but fn only called once - assert(argv0); + argv0 = argv[0]; string cmdline_string; // easier than doing strcats for (int i = 0; i < argc; i++) { @@ -1488,38 +1492,37 @@ void SetArgv(int argc, const char** argv) { cmdline_string += argv[i]; argvs.push_back(argv[i]); } - cmdline = strdup(cmdline_string.c_str()); // another small memory leak - assert(cmdline); + cmdline = cmdline_string.c_str(); // Compute a simple sum of all the chars in argv - for (const char* c = cmdline; *c; c++) + for (string::const_iterator c = cmdline.cbegin(); c != cmdline.cend(); ++c) argv_sum += *c; } const vector& GetArgvs() { return argvs; } -const char* GetArgv() { return cmdline; } -const char* GetArgv0() { return argv0; } +const char* GetArgv() { return cmdline.c_str(); } +const char* GetArgv0() { return argv0.c_str(); } uint32 GetArgvSum() { return argv_sum; } const char* ProgramInvocationName() { // like the GNU libc fn return GetArgv0(); } const char* ProgramInvocationShortName() { // like the GNU libc fn - const char* slash = strrchr(argv0, '/'); + const char* slash = strrchr(argv0.c_str(), '/'); #ifdef OS_WINDOWS if (!slash) slash = strrchr(argv0, '\\'); #endif - return slash ? slash + 1 : argv0; + return slash ? slash + 1 : argv0.c_str(); } void SetUsageMessage(const string& usage) { - if (program_usage != NULL) + if (program_usage.length()) ReportError(DIE, "ERROR: SetUsageMessage() called twice\n"); - program_usage = strdup(usage.c_str()); // small memory leak + program_usage = usage; // small memory leak } const char* ProgramUsage() { - if (program_usage) { - return program_usage; + if (program_usage.length()) { + return program_usage.c_str(); } return "Warning: SetUsageMessage() never called"; } From 13e6f6550f815eb214ad6e8551e7512bdedc3202 Mon Sep 17 00:00:00 2001 From: Massimo Date: Sat, 1 Jun 2013 12:50:09 +0200 Subject: [PATCH 2/4] reduce warnings in Visual Studio Push/pop warning state ignoring deprecated functions. --- src/windows/port.cc | 3 +++ src/windows/port.h | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/src/windows/port.cc b/src/windows/port.cc index fb47698..16fa050 100644 --- a/src/windows/port.cc +++ b/src/windows/port.cc @@ -44,12 +44,15 @@ // These call the windows _vsnprintf, but always NUL-terminate. #if !defined(__MINGW32__) && !defined(__MINGW64__) /* mingw already defines */ +#pragma warning(push) +#pragma warning(disable:4996) // _vsnprintf int safe_vsnprintf(char *str, size_t size, const char *format, va_list ap) { if (size == 0) // not even room for a \0? return -1; // not what C99 says to do, but what windows does str[size-1] = '\0'; return _vsnprintf(str, size-1, format, ap); } +#pragma warning(pop) int snprintf(char *str, size_t size, const char *format, ...) { int r; diff --git a/src/windows/port.h b/src/windows/port.h index fbeccd7..b23bf79 100644 --- a/src/windows/port.h +++ b/src/windows/port.h @@ -65,6 +65,11 @@ extern int GFLAGS_DLL_DECL safe_vsnprintf(char *str, size_t size, #define va_copy(dst, src) (dst) = (src) #endif /* #if !defined(__MINGW32__) && !defined(__MINGW64__) */ +#if defined(_MSC_VER) && _MSC_VER >= 1400 +#pragma warning(push) +#pragma warning(disable:4996) // getenv +#endif/ + inline void setenv(const char* name, const char* value, int) { // In windows, it's impossible to set a variable to the empty string. // We handle this by setting it to "0" and the NUL-ing out the \0. @@ -90,6 +95,7 @@ inline void setenv(const char* name, const char* value, int) { #define strcasecmp _stricmp #if defined(_MSC_VER) && _MSC_VER >= 1400 +#pragma warning(pop) #define strdup _strdup #define unlink _unlink #endif From ee56f2f422c6c9c19b2766d489d1e7f1129afe11 Mon Sep 17 00:00:00 2001 From: Massimo Date: Mon, 3 Jun 2013 19:15:02 +0200 Subject: [PATCH 3/4] fix leak in Windows _putenv_s, available on MS toolchain since VS2005 does not require a buffer allocation. Older MS compilers get no love. --- src/windows/port.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/windows/port.h b/src/windows/port.h index b23bf79..944a5af 100644 --- a/src/windows/port.h +++ b/src/windows/port.h @@ -79,6 +79,13 @@ inline void setenv(const char* name, const char* value, int) { static const char* const kFakeZero = "0"; if (*value == '\0') value = kFakeZero; +#if defined(_MSC_VER) && _MSC_VER >= 1400 + // putenv_s does not need us to malloc anything, therefore no more + // leaks for modern MS toolchain. + _putenv_s(name, value); + if (value == kFakeZero) + *getenv(name) = '\0'; +#else // Apparently the semantics of putenv() is that the input // must live forever, so we leak memory here. :-( const int nameval_len = strlen(name) + 1 + strlen(value) + 1; @@ -90,6 +97,7 @@ inline void setenv(const char* name, const char* value, int) { if (*getenv(name) != '\0') *getenv(name) = '\0'; // works when putenv() copies nameval } +#endif /* defined(_MSC_VER) && _MSC_VER >= 1400 */ } #define strcasecmp _stricmp From f9aa7096a9e761b750699b5de0a8cd871b2c78b1 Mon Sep 17 00:00:00 2001 From: Massimo Date: Mon, 3 Jun 2013 19:15:56 +0200 Subject: [PATCH 4/4] std::string for version_string With this commit, unit tests execute with no leaks on Windows! --- src/gflags.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/gflags.cc b/src/gflags.cc index d4e6a4d..485bb96 100644 --- a/src/gflags.cc +++ b/src/gflags.cc @@ -1532,16 +1532,16 @@ const char* ProgramUsage() { // VersionString() // -------------------------------------------------------------------- -static const char* version_string = NULL; +static string version_string; void SetVersionString(const string& version) { - if (version_string != NULL) + if (version_string.empty() == false) ReportError(DIE, "ERROR: SetVersionString() called twice\n"); - version_string = strdup(version.c_str()); // small memory leak + version_string = version; } const char* VersionString() { - return version_string ? version_string : ""; + return version_string.c_str(); }