* Fix a dangling pointer in template-modifiers (csilvers)

src/template_modifiers.cc

@@ -738,8 +738,8 @@ static struct ModifierWithAlternatives {
                           XSS_WEB_STANDARD, &validate_url_and_css_escape), {} },
 };
 
-static vector<ModifierInfo> g_extension_modifiers;
+static vector<const ModifierInfo*> g_extension_modifiers;
-static vector<ModifierInfo> g_unknown_modifiers;
+static vector<const ModifierInfo*> g_unknown_modifiers;
 
 // Returns whether or not candidate can be safely (w.r.t XSS)
 // used in lieu of our ModifierInfo. This is true iff:
@@ -790,7 +790,9 @@ static bool AddModifierCommon(const char* long_name,
   if (!IsExtensionModifier(long_name))
     return false;
 
-  for (vector<ModifierInfo>::const_iterator mod = g_extension_modifiers.begin();
+  // TODO(csilvers): store in a map or multimap, rather than a vector
+  for (vector<const ModifierInfo*>::const_iterator mod =
+           g_extension_modifiers.begin();
        mod != g_extension_modifiers.end();
        ++mod) {
     // Check if mod has the same name as us.  For modifiers that also take
@@ -799,12 +801,13 @@ static bool AddModifierCommon(const char* long_name,
     // "foo=bar" and "foo=baz" are both valid names.  Note "foo" and
     // "foo=bar" is not valid: foo has no modval, but "foo=bar" does.
     const size_t new_modifier_namelen = strcspn(long_name, "=");
-    const size_t existing_modifier_namelen = strcspn(mod->long_name.c_str(), "=");
+    const size_t existing_modifier_namelen = strcspn((*mod)->long_name.c_str(),
+                                                     "=");
     if (new_modifier_namelen == existing_modifier_namelen &&
-        memcmp(long_name, mod->long_name.c_str(), new_modifier_namelen) == 0) {
+        !memcmp(long_name, (*mod)->long_name.c_str(), new_modifier_namelen)) {
       if (long_name[new_modifier_namelen] == '=' &&
-          mod->long_name[existing_modifier_namelen] == '=' &&
+          (*mod)->long_name[existing_modifier_namelen] == '=' &&
-          mod->long_name != long_name) {
+          (*mod)->long_name != long_name) {
         // It's ok, we're different specializations!
       } else {
         // It's not ok: we have the same name and no good excuse.
@@ -814,9 +817,9 @@ static bool AddModifierCommon(const char* long_name,
   }
 
   g_extension_modifiers.push_back(
-      ModifierInfo(long_name, '\0',
+      new ModifierInfo(long_name, '\0',
-                   xss_safe ? XSS_SAFE : XSS_UNIQUE,
+                       xss_safe ? XSS_SAFE : XSS_UNIQUE,
-                   modifier));
+                       modifier));
   return true;
 }
 
@@ -872,7 +875,7 @@ static void UpdateBestMatch(const char* modname, size_t modname_len,
     }
   } else {
     // In this case, to be a match: we must *not* have a modval.  Our
-    // modname still must match modifno's modname (either short or long).
+    // modname still must match modinfo's modname (either short or long).
     if (modval_len == 0 &&
         ((modname_len == 1 && *modname == candidate_match->short_name) ||
          (modname_len == candidate_match->long_name.size() &&
@@ -892,20 +895,22 @@ const ModifierInfo* FindModifier(const char* modname, size_t modname_len,
   // with the longest longname, since that's the most specialized match.
   const ModifierInfo* best_match = NULL;
   if (modname_len >= 2 && IsExtensionModifier(modname)) {
-    for (vector<ModifierInfo>::const_iterator mod = g_extension_modifiers.begin();
+    for (vector<const ModifierInfo*>::const_iterator mod =
+             g_extension_modifiers.begin();
          mod != g_extension_modifiers.end();
          ++mod) {
       UpdateBestMatch(modname, modname_len, modval, modval_len,
-                      &*mod, &best_match);
+                      *mod, &best_match);
     }
     if (best_match != NULL)
       return best_match;
 
-    for (vector<ModifierInfo>::const_iterator mod = g_unknown_modifiers.begin();
+    for (vector<const ModifierInfo*>::const_iterator mod =
+             g_unknown_modifiers.begin();
          mod != g_unknown_modifiers.end();
          ++mod) {
       UpdateBestMatch(modname, modname_len, modval, modval_len,
-                      &*mod, &best_match);
+                      *mod, &best_match);
     }
     if (best_match != NULL)
       return best_match;
@@ -913,12 +918,12 @@ const ModifierInfo* FindModifier(const char* modname, size_t modname_len,
     // It means "we don't know about this modifier-name."
     string fullname(modname, modname_len);
     if (modval_len) {
-      fullname.append("=");
       fullname.append(modval, modval_len);
     }
-    g_unknown_modifiers.push_back(ModifierInfo(fullname, '\0',
+    // TODO(csilvers): store in a map or multimap, rather than a vector
-                                               XSS_UNIQUE, NULL));
+    g_unknown_modifiers.push_back(new ModifierInfo(fullname, '\0',
-    return &g_unknown_modifiers.back();
+                                                   XSS_UNIQUE, NULL));
+    return g_unknown_modifiers.back();
   } else {
     for (const ModifierWithAlternatives* mod_with_alts = g_modifiers;
          mod_with_alts < g_modifiers + sizeof(g_modifiers)/sizeof(*g_modifiers);

src/tests/template_modifiers_unittest.cc

@@ -37,6 +37,7 @@
 #include <string>
 #include <vector>
 #include "template_modifiers_internal.h"
+#include <ctemplate/template.h>
 #include <ctemplate/template_dictionary.h>
 #include <ctemplate/template_emitter.h>
 #include <ctemplate/template_modifiers.h>
@@ -852,6 +853,41 @@ class TemplateModifiersUnittest {
     ASSERT(modval->modifier_info->modifier ==
            &javascript_escape);
   }
+
+  // This tests for a bug we had where we were returning a pointer into
+  // a vector that became invalid after the vector was resized.
+  static void TestManyUnknownModifiers() {
+    string tpl_str1 = "{{from_name:x-test=4}} sent you a message";
+    const ctemplate::Template* tpl1 = ctemplate::Template::StringToTemplate(
+        tpl_str1, ctemplate::DO_NOT_STRIP);
+
+    string tpl_str2 = "{{from_name:x-test=4}} sent you a message:";
+    string expected_out = "me sent you a message:";
+    // All those new unknown varnames should cause g_unknown_modifiers
+    // to resize.  1111 is an arbitrary large number.
+    for (int i = 0; i < 1111; i++) {
+      tpl_str2.append("{{from_name:x-" + string(i, 't') + "=4}}");
+      expected_out.append("me");
+    }
+    const ctemplate::Template* tpl2 = ctemplate::Template::StringToTemplate(
+        tpl_str2, ctemplate::DO_NOT_STRIP);
+
+    // Even after the resizing, the references to the unknown
+    // modifiers in tpl1 and tpl2 should still be valid.
+    ctemplate::TemplateDictionary dict("test");
+    dict.SetValue("from_name", "me");
+    string out;
+
+    out.clear();
+    tpl1->Expand(&out, &dict);
+    ASSERT_STREQ("me sent you a message", out.c_str());
+    delete tpl1;
+
+    out.clear();
+    tpl2->Expand(&out, &dict);
+    ASSERT_STREQ(expected_out.c_str(), out.c_str());
+    delete tpl2;
+  }
 };
 
 _END_GOOGLE_NAMESPACE_
@@ -878,6 +914,7 @@ int main(int argc, char** argv) {
   TemplateModifiersUnittest::TestAddXssSafeModifier();
   TemplateModifiersUnittest::TestXSSAlternatives();
   TemplateModifiersUnittest::TestDefaultModifiersForContext();
+  TemplateModifiersUnittest::TestManyUnknownModifiers();
 
   printf("DONE\n");
   return 0;

src/windows/ctemplate/template_modifiers.h

@@ -153,8 +153,8 @@ extern CTEMPLATE_DLL_DECL HtmlEscape html_escape;
 class CTEMPLATE_DLL_DECL PreEscape : public TemplateModifier { MODIFY_SIGNATURE_; };
 extern CTEMPLATE_DLL_DECL PreEscape pre_escape;
 
-// Like HtmlEscape but allows HTML entities, <br> tags, <wbr> tags, and
+// Like HtmlEscape but allows HTML entities, <br> tags, <wbr> tags,
-// matched <b> and </b> tags.
+// matched <b> and </b> tags, and matched <i> and </i> tags.
 class CTEMPLATE_DLL_DECL SnippetEscape : public TemplateModifier { MODIFY_SIGNATURE_; };
 extern CTEMPLATE_DLL_DECL SnippetEscape snippet_escape;