Don't allow direct const char* trace value

Source/platform/TraceEvent.h

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 107 matching lines @@
 //     TRACE_EVENT_INSTANT0("SUBSYSTEM", str);  // BAD!
 //     free(str);                   // Trace system now has dangling pointer
 //
 // To avoid this issue with the |name| and |arg_name| parameters, use the
 // TRACE_EVENT_COPY_XXX overloads of the macros at additional runtime overhead.
 // Notes: The category must always be in a long-lived char* (i.e. static const).
 //        The |arg_values|, when used, are always deep copied with the _COPY
 //        macros.
 //
 // When are string argument values copied:
-// const char* arg_values are only referenced by default:
+// - Literal string is always allowed:
+//     TRACE_EVENT1("category", "name", "arg1", "literal string");
+// - Use TRACE_STR_COPY to force copying of a const char*:
 //     TRACE_EVENT1("category", "name",
-//                  "arg1", "literal string is only referenced");
-// Use TRACE_STR_COPY to force copying of a const char*:
+//                  "arg1", TRACE_STR_COPY(string_will_be_copied));
+// - Use TRACE_STR_NO_COPY to explicitly pass a const char* that is ensured
+//   long-lived without copy:
 //     TRACE_EVENT1("category", "name",
-//                  "arg1", TRACE_STR_COPY("string will be copied"));
-// std::string arg_values are always copied:
+//                  "arg1", TRACE_STR_NO_COPY(long_lived_string));
+// - std::string arg_values are always copied:
 //     TRACE_EVENT1("category", "name",
-//                  "arg1", std::string("string will be copied"));
+//                  "arg1", std::string(string_will_be_copied));
 //
 //
 // Thread Safety:
 // A thread safe singleton and mutex are used for thread safety. Category
 // enabled flags are used to limit the performance impact when the system
 // is not enabled.
 //
 // TRACE_EVENT macros first cache a pointer to a category. The categories are
 // statically allocated and safe at all times, even after exit. Fetching a
 // category is protected by the TraceLog::lock_. Multiple threads initializing
@@ skipping 15 matching lines @@
 
 #ifndef TraceEvent_h
 #define TraceEvent_h
 
 #include "platform/EventTracer.h"
 
 #include "wtf/DynamicAnnotations.h"
 #include "wtf/PassRefPtr.h"
 #include "wtf/text/CString.h"
 
-// By default, const char* argument values are assumed to have long-lived scope
-// and will not be copied. Use this macro to force a const char* to be copied.
+// Non-literal const char* argument values are not allowed.
+// Use this macro to force a const char* to be copied.
 #define TRACE_STR_COPY(str) \
-    WebCore::TraceEvent::TraceStringWithCopy(str)
+    WebCore::TraceEvent::TraceStringWrapper<true>(str)
+// And use this macro to explicitly pass a long-lived const char*.
+#define TRACE_STR_NO_COPY(str) \
+    WebCore::TraceEvent::TraceStringWrapper<false>(str)
 
 // By default, uint64 ID argument values are not mangled with the Process ID in
 // TRACE_EVENT_ASYNC macros. Use this macro to force Process ID mangling.
 #define TRACE_ID_MANGLE(id) \
     WebCore::TraceEvent::TraceID::ForceMangle(id)
 
 // By default, pointers are mangled with the Process ID in TRACE_EVENT_ASYNC
 // macros. Use this macro to prevent Process ID mangling.
 #define TRACE_ID_DONT_MANGLE(id) \
     WebCore::TraceEvent::TraceID::DontMangle(id)
@@ skipping 541 matching lines @@
 // Simple union to store various types as unsigned long long.
 union TraceValueUnion {
     bool m_bool;
     unsigned long long m_uint;
     long long m_int;
     double m_double;
     const void* m_pointer;
     const char* m_string;
 };
 
-// Simple container for const char* that should be copied instead of retained.
-class TraceStringWithCopy {
+// Simple container for const char*. The string will be copied or retained
+// according to the Copy template parameter.
+template <bool Copy>

[dsinclair, 2014/06/17 20:47:16]

Can we make the template parameter an enum? CopyString, NoCopyString?

[Xianzhu, 2014/06/17 21:27:35]

Done with a slightly different method using TraceStringWithCopy and
TraceStringWithoutCopy. In this method, the value of the template parameter
actually doesn't matter, while the typedefed types really matters.

+class TraceStringWrapper {
 public:
-    explicit TraceStringWithCopy(const char* str) : m_str(str) { }
-    operator const char* () const { return m_str; }
+    explicit TraceStringWrapper(const char* str) : m_str(str) { }
+    const char* str() const { return m_str; }
 private:
     const char* m_str;
 };
 
 // Define setTraceValue for each allowed type. It stores the type and
 // value in the return arguments. This allows this API to avoid declaring any
 // structures so that it is portable to third_party libraries.
-#define INTERNAL_DECLARE_SET_TRACE_VALUE(actual_type, \
-                                         union_member, \
-                                         value_type_id) \
-    static inline void setTraceValue(actual_type arg, \
-                                     unsigned char* type, \
-                                     unsigned long long* value) { \
+#define INTERNAL_DECLARE_SET_TRACE_VALUE(actualType, argExpression, unionMember, valueTypeId) \
+    static inline void setTraceValue(actualType arg, unsigned char* type, unsigned long long* value) { \
         TraceValueUnion typeValue; \
-        typeValue.union_member = arg; \
-        *type = value_type_id; \
+        typeValue.unionMember = argExpression; \
+        *type = valueTypeId; \
         *value = typeValue.m_uint; \
     }
 // Simpler form for int types that can be safely casted.
-#define INTERNAL_DECLARE_SET_TRACE_VALUE_INT(actual_type, \
-                                             value_type_id) \
-    static inline void setTraceValue(actual_type arg, \
-                                     unsigned char* type, \
-                                     unsigned long long* value) { \
-        *type = value_type_id; \
+#define INTERNAL_DECLARE_SET_TRACE_VALUE_INT(actualType, valueTypeId) \
+    static inline void setTraceValue(actualType arg, unsigned char* type, unsigned long long* value) { \
+        *type = valueTypeId; \
         *value = static_cast<unsigned long long>(arg); \
     }
 
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(unsigned long long, TRACE_VALUE_TYPE_UINT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(unsigned, TRACE_VALUE_TYPE_UINT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(unsigned short, TRACE_VALUE_TYPE_UINT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(unsigned char, TRACE_VALUE_TYPE_UINT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(long long, TRACE_VALUE_TYPE_INT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(int, TRACE_VALUE_TYPE_INT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(short, TRACE_VALUE_TYPE_INT)
 INTERNAL_DECLARE_SET_TRACE_VALUE_INT(signed char, TRACE_VALUE_TYPE_INT)
-INTERNAL_DECLARE_SET_TRACE_VALUE(bool, m_bool, TRACE_VALUE_TYPE_BOOL)
-INTERNAL_DECLARE_SET_TRACE_VALUE(double, m_double, TRACE_VALUE_TYPE_DOUBLE)
-INTERNAL_DECLARE_SET_TRACE_VALUE(const void*, m_pointer,
-                                 TRACE_VALUE_TYPE_POINTER)
-INTERNAL_DECLARE_SET_TRACE_VALUE(const char*, m_string,
-                                 TRACE_VALUE_TYPE_STRING)
-INTERNAL_DECLARE_SET_TRACE_VALUE(const TraceStringWithCopy&, m_string,
-                                 TRACE_VALUE_TYPE_COPY_STRING)
+INTERNAL_DECLARE_SET_TRACE_VALUE(bool, arg, m_bool, TRACE_VALUE_TYPE_BOOL)
+INTERNAL_DECLARE_SET_TRACE_VALUE(double, arg, m_double, TRACE_VALUE_TYPE_DOUBLE)
+INTERNAL_DECLARE_SET_TRACE_VALUE(const TraceStringWrapper<false>&, arg.str(), m_string, TRACE_VALUE_TYPE_STRING)
+INTERNAL_DECLARE_SET_TRACE_VALUE(const TraceStringWrapper<true>&, arg.str(), m_string, TRACE_VALUE_TYPE_COPY_STRING)
 
 #undef INTERNAL_DECLARE_SET_TRACE_VALUE
 #undef INTERNAL_DECLARE_SET_TRACE_VALUE_INT
 
-// WTF::String version of setTraceValue so that trace arguments can be strings.
+template <typename T>
+struct TraceValuePointerTypeAllowed {
+    static const bool allowed = true;
+};
+
+template <>
+struct TraceValuePointerTypeAllowed<char> {
+    static const bool allowed = false;
+};
+
+// Pointer version of SetTraceValue, but const char* is not allowed.
+template <typename T>
+static inline void setTraceValue(const T* arg, unsigned char* type, unsigned long long* value)
+{
+    COMPILE_ASSERT(TraceValuePointerTypeAllowed<T>::allowed, invalid_pointer_type);
+    TraceValueUnion typeValue;
+    typeValue.m_pointer = arg;
+    *type = TRACE_VALUE_TYPE_POINTER;
+    *value = typeValue.m_uint;
+}
+
+// Allow literal string.
+// FIXME: This will also allow passing local char arrays. Is it possible to avoid?
+template <size_t N>
+static inline void setTraceValue(const char (&arg)[N], unsigned char* type, unsigned long long* value)

[dsinclair, 2014/06/17 20:47:16]

This is going to end up stamping out a function for every length of string that
we pass into trace event. That seems like it could get expensive?

[Xianzhu, 2014/06/17 21:27:35]

There should be no difference before and after the change, because the functions
are inlined. Each setTraceValue call site is expanded to 2 or 3 assignments
despite this change (TRACE_STR_NO_COPY() and TRACE_STR_COPY() are just no-op in
optimized code).

+{
+    setTraceValue(TRACE_STR_NO_COPY(arg), type, value);
+}
+
+// WTF::CString version of setTraceValue so that trace arguments can be strings.
 static inline void setTraceValue(const WTF::CString& arg, unsigned char* type, unsigned long long* value)
 {
     TraceValueUnion typeValue;
     typeValue.m_string = arg.data();
     *type = TRACE_VALUE_TYPE_COPY_STRING;
     *value = typeValue.m_uint;
 }
 
 static inline void setTraceValue(ConvertableToTraceFormat*, unsigned char* type, unsigned long long*)
 {
@@ skipping 208 matching lines @@
     const char* m_categoryGroup;
     const char* m_name;
     IDType m_id;
 };
 
 } // namespace TraceEvent
 
 } // namespace WebCore
 
 #endif