Issue 5174 - Allow '{' and '}' in selectors

compiled/filter/ElemHideBase.cpp

Index: compiled/filter/ElemHideBase.cpp
===================================================================
--- a/compiled/filter/ElemHideBase.cpp
+++ b/compiled/filter/ElemHideBase.cpp
@@ -10,16 +10,18 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <cstring>
+
 #include "ElemHideBase.h"
 #include "../StringScanner.h"
 
 namespace
 {
   void NormalizeWhitespace(DependentString& text, String::size_type& domainsEnd,
       String::size_type& selectorStart)
   {
@@ -98,40 +100,83 @@
   // Selector part
 
   // Selector shouldn't be empty
   seenSpaces |= scanner.skip(u' ');
   if (scanner.done())
     return Type::UNKNOWN;
 
   data.mSelectorStart = scanner.position() + 1;
-  while (!scanner.done())
-  {
-    switch (scanner.next())
-    {
-      case u'{':
-      case u'}':
-        return Type::UNKNOWN;
-    }
-  }
 
   // We are done validating, now we can normalize whitespace and the domain part
   if (seenSpaces)
     NormalizeWhitespace(text, data.mDomainsEnd, data.mSelectorStart);
   DependentString(text, 0, data.mDomainsEnd).toLower();
 
   if (exception)
     return Type::ELEMHIDEEXCEPTION;
 
   if (text.find(u"[-abp-properties="_str, data.mSelectorStart) != text.npos)
     return Type::ELEMHIDEEMULATION;
 
   return Type::ELEMHIDE;
 }
 
+namespace
+{
+
+static constexpr String::value_type OPENING_CURLY_REPLACEMENT[] = u"\\x7B ";
+static constexpr String::value_type CLOSING_CURLY_REPLACEMENT[] = u"\\x7D ";
+static constexpr String::size_type CURLY_REPLACEMENT_SIZE = sizeof(OPENING_CURLY_REPLACEMENT) / sizeof(OPENING_CURLY_REPLACEMENT[0]) - 1;

[sergei, 2017/10/18 14:01:37]

What about a constexpr function calculating the length of the string instead of
a global variable? It will be the same but resistant to any change of the
strings, which is pretty unlikely though.

+
+OwnedString EscapeCurlies(String::size_type replacementCount,
+                          const DependentString& str)
+{
+  OwnedString result(str.length() + replacementCount * (CURLY_REPLACEMENT_SIZE - 1));
+
+  String::value_type* current = result.data();
+  for (String::size_type i = 0; i < str.length(); i++)
+  {
+    switch(str[i])
+    {
+    case u'}':
+      std::memcpy(current, CLOSING_CURLY_REPLACEMENT,
+                  sizeof(String::value_type) * CURLY_REPLACEMENT_SIZE);
+      current += CURLY_REPLACEMENT_SIZE;

[sergei, 2017/10/18 14:01:37]

What about moving of memcpy into the String class? If we do it, I think it
should be rather in another commit. Despite it can introduce some overhead it
can be very useful for other cases.

String::size_type currentPos = 0;
...
currentPos += result.write_at(currentPos, CLOSING_CURLY_REPLACEMENT,
CURLY_REPLACEMENT_SIZE);

// returns the number of written chars
size_type String::write_at(size_type pos, const value_type* data, size_type
count)
{
  count = ... ; // ensure that it does not go beyond the length
  if (mBuf != nullptr && data != nullptr && count > 0)
    std::memcpy(current, data, sizeof(value_type) * count);
  return count;
}

+      break;
+    case u'{':
+      std::memcpy(current, OPENING_CURLY_REPLACEMENT,
+                  sizeof(String::value_type) * CURLY_REPLACEMENT_SIZE);
+      current += CURLY_REPLACEMENT_SIZE;
+      break;
+    default:
+      *current = str[i];
+      current++;
+      break;
+    }
+  }
+
+  return result;
+}
+
+}
+
+OwnedString ElemHideBase::GetSelector() const
+{
+  DependentString selector = mData.GetSelector(mText);
+  String::size_type replacementCount = 0;
+  for (String::size_type i = 0; i < selector.length(); i++)
+    if (selector[i] == '}' || selector[i] == '{')
+      replacementCount++;
+  if (replacementCount)
+    return EscapeCurlies(replacementCount, selector);
+
+  return OwnedString(selector);
+}
+
 OwnedString ElemHideBase::GetSelectorDomain() const
 {
   /* TODO this is inefficient */
   OwnedString result;
   if (mDomains)
   {
     for (const auto& item : *mDomains)
     {