Detecting and fixing stringprintf.h format bugs

The print functions in stringprintf.h were not annotated for /analyze so 13 Windows specific format-string bugs accumulated. This annotates the functions so that the /analyze builder will find the problems and fixes the bugs. R=thestig@chromium.org,wfh@chromium.org,jam@chromium.org Skipping wstring presubmit checks - no new wstring usage is added NOPRESUBMIT=true BUG=427616 Review URL: https://codereview.chromium.org/1372153002 Cr-Commit-Position: refs/heads/master@{#352659}
qweri0p · Oct 6, 2015 · 5604a11 · 5604a11
1 parent a45aa1e
commit 5604a11
Show file tree

Hide file tree

Showing 10 changed files with 56 additions and 32 deletions.
diff --git a/base/files/file_path.h b/base/files/file_path.h
@@ -124,6 +124,15 @@
 #define FILE_PATH_USES_WIN_SEPARATORS
 #endif  // OS_WIN
 
+// To print path names portably use PRIsFP (based on PRIuS and friends from
+// C99 and format_macros.h) like this:
+// base::StringPrintf("Path is %" PRIsFP ".\n", path.value().c_str());
+#if defined(OS_POSIX)
+#define PRIsFP "s"
+#elif defined(OS_WIN)
+#define PRIsFP "ls"
+#endif  // OS_WIN
+
 namespace base {
 
 class Pickle;

diff --git a/base/memory/shared_memory_win.cc b/base/memory/shared_memory_win.cc
@@ -146,7 +146,7 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
     // So, we generate a random name when we need to enforce read-only.
     uint64_t rand_values[4];
     RandBytes(&rand_values, sizeof(rand_values));
-    name_ = StringPrintf(L"CrSharedMem_%016x%016x%016x%016x",
+    name_ = StringPrintf(L"CrSharedMem_%016llx%016llx%016llx%016llx",
                          rand_values[0], rand_values[1],
                          rand_values[2], rand_values[3]);
   }

diff --git a/base/strings/stringprintf.h b/base/strings/stringprintf.h
@@ -11,37 +11,52 @@
 
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
+#include "build/build_config.h"
+
+#ifdef COMPILER_MSVC
+// For _Printf_format_string_.
+#include <sal.h>
+#else
+// For nacl builds when sal.h is not available.
+#define _Printf_format_string_
+#endif
 
 namespace base {
 
 // Return a C++ string given printf-like input.
-BASE_EXPORT std::string StringPrintf(const char* format, ...)
+BASE_EXPORT std::string StringPrintf(_Printf_format_string_ const char* format,
+                                     ...)
     PRINTF_FORMAT(1, 2) WARN_UNUSED_RESULT;
 #if defined(OS_WIN)
-BASE_EXPORT std::wstring StringPrintf(const wchar_t* format, ...)
-    WPRINTF_FORMAT(1, 2) WARN_UNUSED_RESULT;
+BASE_EXPORT std::wstring StringPrintf(
+    _Printf_format_string_ const wchar_t* format,
+    ...) WPRINTF_FORMAT(1, 2) WARN_UNUSED_RESULT;
 #endif
 
 // Return a C++ string given vprintf-like input.
 BASE_EXPORT std::string StringPrintV(const char* format, va_list ap)
     PRINTF_FORMAT(1, 0) WARN_UNUSED_RESULT;
 
 // Store result into a supplied string and return it.
-BASE_EXPORT const std::string& SStringPrintf(std::string* dst,
-                                             const char* format, ...)
-    PRINTF_FORMAT(2, 3);
+BASE_EXPORT const std::string& SStringPrintf(
+    std::string* dst,
+    _Printf_format_string_ const char* format,
+    ...) PRINTF_FORMAT(2, 3);
 #if defined(OS_WIN)
-BASE_EXPORT const std::wstring& SStringPrintf(std::wstring* dst,
-                                              const wchar_t* format, ...)
-    WPRINTF_FORMAT(2, 3);
+BASE_EXPORT const std::wstring& SStringPrintf(
+    std::wstring* dst,
+    _Printf_format_string_ const wchar_t* format,
+    ...) WPRINTF_FORMAT(2, 3);
 #endif
 
 // Append result to a supplied string.
-BASE_EXPORT void StringAppendF(std::string* dst, const char* format, ...)
-    PRINTF_FORMAT(2, 3);
+BASE_EXPORT void StringAppendF(std::string* dst,
+                               _Printf_format_string_ const char* format,
+                               ...) PRINTF_FORMAT(2, 3);
 #if defined(OS_WIN)
-BASE_EXPORT void StringAppendF(std::wstring* dst, const wchar_t* format, ...)
-    WPRINTF_FORMAT(2, 3);
+BASE_EXPORT void StringAppendF(std::wstring* dst,
+                               _Printf_format_string_ const wchar_t* format,
+                               ...) WPRINTF_FORMAT(2, 3);
 #endif
 
 // Lower-level routine that takes a va_list and appends to a specified

diff --git a/chrome/browser/sync_file_system/drive_backend/local_to_remote_syncer.cc b/chrome/browser/sync_file_system/drive_backend/local_to_remote_syncer.cc
@@ -148,9 +148,9 @@ void LocalToRemoteSyncer::RunPreflight(scoped_ptr<SyncTaskToken> token) {
   } else if (active_ancestor_path != path) {
     if (!active_ancestor_path.AppendRelativePath(path, &missing_entries)) {
       NOTREACHED();
-      token->RecordLog(base::StringPrintf(
-          "Detected invalid ancestor: %s",
-          active_ancestor_path.value().c_str()));
+      token->RecordLog(
+          base::StringPrintf("Detected invalid ancestor: %" PRIsFP,
+                             active_ancestor_path.value().c_str()));
       SyncTaskManager::NotifyTaskDone(token.Pass(), SYNC_STATUS_FAILED);
       return;
     }

diff --git a/chrome/common/extensions/api/storage/storage_schema_manifest_handler.cc b/chrome/common/extensions/api/storage/storage_schema_manifest_handler.cc
@@ -49,13 +49,13 @@ policy::Schema StorageSchemaManifestHandler::GetSchema(
   }
   file = extension->path().AppendASCII(path);
   if (!base::PathExists(file)) {
-    *error =
-        base::StringPrintf("File does not exist: %s", file.value().c_str());
+    *error = base::StringPrintf("File does not exist: %" PRIsFP,
+                                file.value().c_str());
     return policy::Schema();
   }
   std::string content;
   if (!base::ReadFileToString(file, &content)) {
-    *error = base::StringPrintf("Can't read %s", file.value().c_str());
+    *error = base::StringPrintf("Can't read %" PRIsFP, file.value().c_str());
     return policy::Schema();
   }
   return policy::Schema::Parse(content, error);

diff --git a/content/common/sandbox_win.cc b/content/common/sandbox_win.cc
@@ -224,7 +224,7 @@ void AddGenericDllEvictionPolicy(sandbox::TargetPolicy* policy) {
 // Returns the object path prepended with the current logon session.
 base::string16 PrependWindowsSessionPath(const base::char16* object) {
   // Cache this because it can't change after process creation.
-  static uintptr_t s_session_id = 0;
+  static DWORD s_session_id = 0;
   if (s_session_id == 0) {
     HANDLE token;
     DWORD session_id_length;
@@ -238,7 +238,7 @@ base::string16 PrependWindowsSessionPath(const base::char16* object) {
       s_session_id = session_id;
   }
 
-  return base::StringPrintf(L"\\Sessions\\%d%ls", s_session_id, object);
+  return base::StringPrintf(L"\\Sessions\\%lu%ls", s_session_id, object);
 }
 
 // Checks if the sandbox should be let to run without a job object assigned.

diff --git a/ipc/handle_win.cc b/ipc/handle_win.cc
@@ -48,7 +48,7 @@ bool ParamTraits<HandleWin>::Read(const Message* m,
 
 // static
 void ParamTraits<HandleWin>::Log(const param_type& p, std::string* l) {
-  l->append(base::StringPrintf("0x%X", p.get_handle()));
+  l->append(base::StringPrintf("0x%p", p.get_handle()));
   l->append(base::IntToString(p.get_permissions()));
 }
 

diff --git a/ipc/ipc_message_utils.cc b/ipc/ipc_message_utils.cc
@@ -966,7 +966,7 @@ bool ParamTraits<HANDLE>::Read(const Message* m,
 }
 
 void ParamTraits<HANDLE>::Log(const param_type& p, std::string* l) {
-  l->append(base::StringPrintf("0x%X", p));
+  l->append(base::StringPrintf("0x%p", p));
 }
 
 void ParamTraits<LOGFONT>::Write(Message* m, const param_type& p) {

diff --git a/mojo/fetcher/network_fetcher.cc b/mojo/fetcher/network_fetcher.cc
@@ -90,8 +90,8 @@ void NetworkFetcher::RecordCacheToURLMapping(const base::FilePath& path,
   base::FilePath map_path = temp_dir.AppendASCII(map_name);
 
   // TODO(eseidel): Paths or URLs with spaces will need quoting.
-  std::string map_entry =
-      base::StringPrintf("%s %s\n", path.value().c_str(), url.spec().c_str());
+  std::string map_entry = base::StringPrintf(
+      "%" PRIsFP " %s\n", path.value().c_str(), url.spec().c_str());
   // TODO(eseidel): AppendToFile is missing O_CREAT, crbug.com/450696
   if (!PathExists(map_path)) {
     base::WriteFile(map_path, map_entry.data(),

diff --git a/sync/engine/model_type_worker.cc b/sync/engine/model_type_worker.cc
@@ -178,9 +178,9 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
   }
 
   DVLOG(1) << ModelTypeToString(type_) << ": "
-           << base::StringPrintf(
-                  "Delivering %zd applicable and %zd pending updates.",
-                  response_datas.size(), pending_updates.size());
+           << base::StringPrintf("Delivering %" PRIuS " applicable and %" PRIuS
+                                 " pending updates.",
+                                 response_datas.size(), pending_updates.size());
 
   // Forward these updates to the model thread so it can do the rest.
   model_type_processor_->OnUpdateReceived(data_type_state_, response_datas,
@@ -411,9 +411,9 @@ void ModelTypeWorker::OnCryptographerUpdated() {
 
   if (new_encryption_key || response_datas.size() > 0) {
     DVLOG(1) << ModelTypeToString(type_) << ": "
-             << base::StringPrintf(
-                    "Delivering encryption key and %zd decrypted updates.",
-                    response_datas.size());
+             << base::StringPrintf("Delivering encryption key and %" PRIuS
+                                   " decrypted updates.",
+                                   response_datas.size());
     model_type_processor_->OnUpdateReceived(data_type_state_, response_datas,
                                             UpdateResponseDataList());
   }