Issue 1115 - Some yahoo page not correctly shown on IE8 when ABP enabled

src/plugin/PluginWbPassThrough.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 163 matching lines @@
     return ContentType::CONTENT_TYPE_XMLHTTPREQUEST;
   }
 
   return ContentType::CONTENT_TYPE_OTHER;
 }
 
 ContentType WBPassthruSink::GetContentTypeFromURL(const std::wstring& src)
 {
   std::wstring schemeAndHierarchicalPart = GetSchemeAndHierarchicalPart(src);
   auto contentType = GetContentTypeFromString(schemeAndHierarchicalPart);
-  if (AdblockPlus::IE::InstalledMajorVersion() == 8 &&
-    contentType == ContentType::CONTENT_TYPE_OTHER)

[Eric, 2015/02/12 17:24:41]

I don't think it matters all that much, but it would be a little better to test
content type first and check the IE version only if we need to.

[sergei, 2015/02/13 15:21:56]

Changed.

-  {
-    ForEachQueryStringParameter(GetQueryString(src),

[Oleksandr, 2015/02/13 03:32:16]

Why can't we just use strtok here? The code would be much simpler, IMHO.

[sergei, 2015/02/13 15:21:56]

strtok is unsafe and I would say the using of any function from the family is
not simpler, https://msdn.microsoft.com/en-us/library/ftsafwz3.aspx

[Eric, 2015/02/13 16:33:43]

For the reasons that strtok won't work here, see
http://stackoverflow.com/questions/4031075/strtok-function-thread-safety

Microsoft doesn't supply any variation of strtok_r, which might work.

[Oleksandr, 2015/02/16 06:59:28]

strotok_s would actually work good here. It has a context parameter instead of
static buffer used in strtok. In fact, strotok in MSVC is thread safe too:
https://social.msdn.microsoft.com/Forums/vstudio/en-US/963aac07-da1a-4612-be4...
So strotok_s is thread safe and reentrant. 
With that we won't need all the new methods and test files and current code
could look something like [NOT TESTED]:

std::wstring queryString = GetQueryString(src);
contentType = ContentType::CONTENT_TYPE_OTHER;
token = wcstok_s(src, L"&=", &next_token);
while (token != nullptr)
{
  contentType = GetContentTypeFromString(token);
  if (contentType != ContentType::CONTENT_TYPE_OTHER)
  {
     return contentType;
  }
  token = wcstok_s(NULL, L"&=", &next_token);
}

This would be much more straightforward and better than the current approach
IMO, which takes a bit time to comprehend.

[sergei, 2015/02/16 10:54:33]

I agree it looks good, not as I expected.

[Eric, 2015/02/16 18:18:07]

There are two underlying observations here. The first is that there's a library
function, albeit a non-standard Microsoft-specific one, that can split tokens.
The second is that, _for this particular function_, there's no need to
distinguish between name and value in a URL parameter string.

Having seen the new code, I greatly prefer the use of a function iterator and an
inline lambda. It find it easier to understand that the new commingled version.
Separating syntax and semantics is almost always the right thing to do, in no
small part because large numbers of security problems come from bad parsing (the
syntax part), and because it's much easier to validate parsing code when it's
not commingled with other concerns.

However, the previous version of the code could have been simpler. The
particular algorithm used makes no distinction between name and value. Hence it
could have been written with 'ForEachToken' rather than
'ForEachQueryStringParameter'. In that case the body of the lambda would be much
smaller, something like this:

    if (token.empty()) return true;
    contentType = GetContentTypeFromString(token);
    return contentType == ContentType::CONTENT_TYPE_OTHER;

The other part, that about 'strtok_s', is a viable way of rewriting
'ForEachToken'. I wasn't fond of its previous implementation, although it was
adequate. Using the library function would clean it up.

[sergei, 2015/02/20 11:17:03]

Good, but what does it give us? We use a lot of Windows API, yet one
Microsoft-specific should not cause any more trouble. And it looks reasonable to
start with the value and then try with the name but it's too premature
assumption, so I've removed it without any comments.
I completely support it!
Actually, I find the previous implementation a bit more complicated than the
current because the previous one requires at least to read the description of
the function to understand the aim of the return value of the lambda. It's a
design flaw, it's better to use enum instead of bool there, but all these things
only complicate it, I guess.

I would not say that the using of any function from `strtok` family is easier
and is good for functions like `ForEachToken`. Pay attention that we pass
"immutable" object to `ForEachToken` (const string&) but `strtok` modifies it.
In this particular case it's clear and we are modifying the temporary object,
but, as I said, I find all such peculiarities only complicating the code.
In addition, the previous implementation of `ForEachToken` works with iterators
which is safer than working with the raw pointers.

So, I would like to keep the current version (with `strtok`) but if you want
it's completely fine for me to implement `ForEachToken` which works with a set
of delimiters (but not based on `strtok`).

-    [&contentType](const std::wstring& name, const std::wstring& value)->bool
-    {
-      if (!value.empty())
-      {
-        contentType = GetContentTypeFromString(value);
-        if (contentType != ContentType::CONTENT_TYPE_OTHER)
-        {
-          return false;
-        }
-      }
-      contentType = GetContentTypeFromString(name);
+  if (contentType == ContentType::CONTENT_TYPE_OTHER &&
+    AdblockPlus::IE::InstalledMajorVersion() == 8)
+  {
+    std::wstring queryString = GetQueryString(src);
+    wchar_t* nextToken = nullptr;
+    const wchar_t* token = wcstok_s(&queryString[0], L"&=", &nextToken);
+    while (token != nullptr)
+    {
+      contentType = GetContentTypeFromString(token);
       if (contentType != ContentType::CONTENT_TYPE_OTHER)
       {
-        return false;
+         return contentType;
       }
-      return true;
-    });
+      token = wcstok_s(nullptr, L"&=", &nextToken);
+    }
   }
   return contentType;
 }
 
 ContentType WBPassthruSink::GetContentType(const CString& mimeType, const std::wstring& domain, const std::wstring& src)
 {
   // No referer or mime type
   // BINDSTRING_XDR_ORIGIN works only for IE v8+
   if (mimeType.IsEmpty() && domain.empty() && AdblockPlus::IE::InstalledMajorVersion() >= 8)
   {
@@ skipping 231 matching lines @@
   }
 
   return OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, m_spInternetProtocol);
 }
 
 STDMETHODIMP WBPassthru::Read(/* [in, out] */ void *pv,/* [in] */ ULONG cb,/* [out] */ ULONG *pcbRead)
 {
   WBPassthruSink* pSink = GetSink();
   return pSink->OnRead(pv, cb, pcbRead);
 }