Run a single FilterEngine instance in a separate process

AdblockPlusEngine/main.cpp

 #include <AdblockPlus.h>
 #include <iostream>
+#include <Lmcons.h>
 #include <ShlObj.h>
 #include <sstream>
 #include <vector>
 #include <Windows.h>
 #include <Sddl.h>

[Wladimir Palant, 2013/05/16 11:52:16]

I think that the usual order of includes is from generic towards more specific.
I would order these like that:

#include <iostream>
#include <sstream>
#include <vector>

#include <Sddl.h>
#include <ShlObj.h>
#include <Windows.h>

#include <AdblockPlus.h>

In other words, default STL headers first, then Windows headers, then our
generic headers and finally the header for this particular file.

Mozilla's style guide doesn't seem to have a rule on that and I don't have
strong feelings about that - but IMHO this order helps create overview (unlike
strict alphabetic order).

[Felix Dahlke, 2013/05/17 09:11:36]

We're using alphabetical in most places, but it's not consistent across files.
I've ordered headers the way you prefer it for a few years, always ended up
being messy over time, so I moved to alphabetical at some point.

I think we should either address this across all files or just let it be a bit
inconsistent, which I don't mind.

 
 namespace
 {
-  const std::wstring pipeName = L"\\\\.\\pipe\\adblockplusengine";
+  std::wstring GetUserName()
+  {
+    const DWORD maxLength = UNLEN + 1;
+    std::auto_ptr<wchar_t> buffer(new wchar_t[maxLength]);
+    DWORD length = maxLength;
+    if (!::GetUserName(buffer.get(), &length))
+    {
+      std::stringstream stream;
+      stream << "Failed to get the current user's name (Error code: " << GetLastError() << ")";
+      throw std::runtime_error("Failed to get the current user's name");
+    }
+    return std::wstring(buffer.get(), length);
+  }
+
+  const std::wstring pipeName = L"\\\\.\\pipe\\adblockplusengine_" + GetUserName();
   const int bufferSize = 1024;
   std::auto_ptr<AdblockPlus::FilterEngine> filterEngine;
-  HANDLE filterEngineMutex;
 
   class AutoHandle
   {
   public:
     AutoHandle()
     {
     }
 
     AutoHandle(HANDLE handle) : handle(handle)
     {
     }
 
     ~AutoHandle()
     {
       CloseHandle(handle);
     }
 
     HANDLE get()
     {
       return handle;
     }
 
   private:
     HANDLE handle;
-    static int references;

[Wladimir Palant, 2013/05/16 11:52:16]

Unused member?

[Felix Dahlke, 2013/05/23 12:31:20]

Done.

 
     AutoHandle(const AutoHandle& autoHandle);
     AutoHandle& operator=(const AutoHandle& autoHandle);
   };
 
   void Log(const std::string& message)
   {
     // TODO: Log to a log file
     MessageBoxA(0, ("AdblockPlusEngine: " + message).c_str(), "", MB_OK);
   }
 
   void LogLastError(const std::string& message)
   {
     std::stringstream stream;
     stream << message << " (Error code: " << GetLastError() << ")";
     Log(stream.str());
   }
 
   void LogException(const std::exception& exception)
   {
     Log(std::string("An exception occurred: ") + exception.what());
   }
 
   std::string MarshalStrings(const std::vector<std::string>& strings)
   {
     // TODO: This is some pretty hacky marshalling, replace it with something more robust

[Wladimir Palant, 2013/05/16 11:52:16]

That's an understatement :)

     std::string marshalledStrings;
     for (std::vector<std::string>::const_iterator it = strings.begin(); it != strings.end(); it++)
       marshalledStrings += *it + ';';
     return marshalledStrings;
   }
 
   std::vector<std::string> UnmarshalStrings(const std::string& message)
   {
     std::stringstream stream(message);
     std::vector<std::string> strings;
     std::string string;
     while (std::getline(stream, string, ';'))
         strings.push_back(string);
     return strings;
   }
 
-  std::string ToString(std::wstring value)
-  {
-    int size = WideCharToMultiByte(CP_UTF8, 0, value.c_str(), -1, 0, 0, 0, 0);

[Wladimir Palant, 2013/05/16 11:52:16]

WideCharToMultiByte() will return 0 on error, this needs to be handled. Also,
please pass the string size explicitly here, not -1. You might actually get a
string with a null character in it...

[Felix Dahlke, 2013/05/23 12:31:20]

Done. Learnt something here: I thought std::string::length() would just count
the bytes until the first binary 0 like strlen, but it does in fact return the
full length of strings with 0 in the middle.

-    char* converted = new char[size];

[Wladimir Palant, 2013/05/16 11:52:16]

std::auto_ptr please, don't delete manually.

[Felix Dahlke, 2013/05/23 12:31:20]

Done.

-    WideCharToMultiByte(CP_UTF8, 0, value.c_str(), -1, converted, size, 0, 0);
-    std::string string(converted);

[Wladimir Palant, 2013/05/16 11:52:16]

string(converted, size) please - you have an explicit size. Please consider
writing directly to std::string buffer as done in
AdblockPlus::Utils::ToUTF8String(). Or maybe just using
AdblockPlus::Utils::ToUTF8String().

[Felix Dahlke, 2013/05/17 09:11:36]

Writing directly to the string buffer is still not something I'd recommend. In
C++11, strings are guaranteed to be stored continuously. In earlier standards,
they are not, it's just that most compilers happen to do do it. The additional
copy doesn't hurt us here.

But that just as an aside, I'll guess we'll use ToUTF8String().

[Felix Dahlke, 2013/05/23 12:31:20]

Decided to keep the function after all. ToUTF8String is an internal utility of
libadblockplus, not really part of it's API. Wouldn't want to expose it just to
make things easier for us here. Objections?

[Wladimir Palant, 2013/05/23 14:25:01]

No real objections but the function name should at least indicate that a charset
conversion is happening here - so I would prefer ToUtf8String() and the same
contents as in libadblockplus.

[Felix Dahlke, 2013/05/23 19:10:44]

Just copied the function from libadblockplus, although I think it does a few
unnecessary things and I'm not a fan of writing to the internal buffer of an
std::string. But nothing I can't live with.

-    delete converted;
-    return string;
+  std::string ToUtf8String(std::wstring str)
+  {
+    size_t length = str.size();
+    if (length == 0)
+      return std::string();
+
+    DWORD utf8StringLength = WideCharToMultiByte(CP_UTF8, 0, str.c_str(), length, 0, 0, 0, 0);
+    if (utf8StringLength == 0)
+      throw std::runtime_error("Failed to determine the required buffer size");
+
+    std::string utf8String(utf8StringLength, '\0');
+    WideCharToMultiByte(CP_UTF8, 0, str.c_str(), length, &utf8String[0], utf8StringLength, 0, 0);
+    return utf8String;
   }
 
   std::string ReadMessage(HANDLE pipe)
   {
     std::stringstream stream;
     std::auto_ptr<char> buffer(new char[bufferSize]);
-    bool hasError;
-    do
+    bool doneReading = false;
+    while (!doneReading)
     {
       DWORD bytesRead;
-      hasError = !ReadFile(pipe, buffer.get(), bufferSize * sizeof(char), &bytesRead, 0);
-      if (hasError && GetLastError() != ERROR_MORE_DATA)
+      if (ReadFile(pipe, buffer.get(), bufferSize * sizeof(char), &bytesRead, 0))
+        doneReading = true;
+      else if (GetLastError() != ERROR_MORE_DATA)
       {
         std::stringstream stream;
         stream << "Error reading from pipe: " << GetLastError();
         throw std::runtime_error(stream.str());
       }
       stream << std::string(buffer.get(), bytesRead);
-    } while (hasError);

[Wladimir Palant, 2013/05/16 11:52:16]

The flow is rather hard to follow here, particularly with a hasError variable
that doesn't indicate error. How about:

bool done = false;
while (!done)
{
  if (ReadFile(...))
    done = true;
  else if (GetLastError() != ERROR_MORE_DATA)
    throw std::runtime_error(...);

  stream << std::string(...);
}

[Felix Dahlke, 2013/05/23 12:31:20]

Done.

+    }
     return stream.str();
   }
 
   void WriteMessage(HANDLE pipe, const std::string& message)
   {
     DWORD bytesWritten;
     if (!WriteFile(pipe, message.c_str(), message.length(), &bytesWritten, 0)) 
       throw std::runtime_error("Failed to write to pipe");

[Wladimir Palant, 2013/05/16 11:52:16]

What if bytesWritten < message.length()? According to documentation, this can
happen when "writing to a non-blocking, byte-mode pipe handle with insufficient
buffer space". I guess that our pipe handle is blocking and we can safely ignore
that scenario?

[Felix Dahlke, 2013/05/17 09:11:36]

Yes, it's blocking. And writing long strings works (e.g. element hiding rules).
We still have to pass bytesWritten in unfortunately.

   }
 
   std::string HandleRequest(const std::vector<std::string>& strings)
   {
     std::string procedureName = strings[0];
     if (procedureName == "Matches")
       return filterEngine->Matches(strings[1], strings[2], strings[3]) ? "1" : "0";
     if (procedureName == "GetElementHidingSelectors")
       return MarshalStrings(filterEngine->GetElementHidingSelectors(strings[1]));
     return "";
   }
 
   DWORD WINAPI ClientThread(LPVOID param)
   {
     HANDLE pipe = static_cast<HANDLE>(param);
 
     try
     {
       std::string message = ReadMessage(pipe);
       std::vector<std::string> strings = UnmarshalStrings(message);
-      WaitForSingleObject(filterEngineMutex, INFINITE);

[Wladimir Palant, 2013/05/16 11:52:16]

Doesn't this duplicate the functionality of mutexes we already have in
libadblockplus? JS calls are already serialized.

[Felix Dahlke, 2013/05/23 12:31:20]

Done.

       std::string response = HandleRequest(strings);
-      ReleaseMutex(filterEngineMutex);
       WriteMessage(pipe, response);
     }
     catch (const std::exception& e)
     {
       LogException(e);
     }
 
+    // TODO: Keep the pipe open until the client disconnects
     FlushFileBuffers(pipe);
     DisconnectNamedPipe(pipe);
     CloseHandle(pipe);

[Wladimir Palant, 2013/05/16 11:52:16]

Opening a new connection for each calls sounds like quite some overhead - it
would make more sense to have the clients establish a connection when they
initialize and keep it up indefinitely while exchanging messages back and forth.
The engine would shut down then the last client connection disappears.

[Felix Dahlke, 2013/05/17 09:11:36]

I previously read that there is a limit to the number of named pipe instances.
However, dug a bit deeper and it appears to be unlimited. So yeah, we'll keep
them open.

[Felix Dahlke, 2013/05/23 12:31:20]

Began to work on this, but it's a bit tricky and I'd like to do it after the
merge to not hold things back. Added a TODO.

     return 0;
   }
-}
-
-bool IsWindowsVistaOrLater()

[Wladimir Palant, 2013/05/16 11:52:16]

Any reason why some functions are inside an empty namespace while others aren't?
I cannot really see the logic here.

[Felix Dahlke, 2013/05/17 09:11:36]

Forgot to move it there.

-{
-  OSVERSIONINFOEX osvi;
-  ZeroMemory(&osvi, sizeof(OSVERSIONINFOEX));
-  osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
-  GetVersionEx(reinterpret_cast<LPOSVERSIONINFO>(&osvi));
-  return osvi.dwMajorVersion >= 6;
+
+  bool IsWindowsVistaOrLater()
+  {
+    OSVERSIONINFOEX osvi;
+    ZeroMemory(&osvi, sizeof(OSVERSIONINFOEX));
+    osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
+    GetVersionEx(reinterpret_cast<LPOSVERSIONINFO>(&osvi));
+    return osvi.dwMajorVersion >= 6;
+  }
 }
 
 std::wstring GetAppDataPath()
 {
-  wchar_t appDataPath[MAX_PATH];
+  std::wstring appDataPath;
   if (IsWindowsVistaOrLater())
   {
-    WCHAR* dataPath;
-    if (FAILED(SHGetKnownFolderPath(FOLDERID_LocalAppDataLow, 0, 0, &dataPath)))

[Wladimir Palant, 2013/05/16 11:52:16]

I don't think that statically compiling against SHGetKnownFolderPath will work -
that function doesn't exist on Windows XP, consequently loading the application
will fail. You need to work with LoadLibrary() and GetProcAddress() here.

Also, we might want to try passing KF_FLAG_CREATE - not sure it helps much
however if the folder doesn't exist since we are running at low integrity.

[Felix Dahlke, 2013/05/23 12:31:20]

I'll leave this to Oleksandr, since he's working on getting the plugin to work
on XP.

[Oleksandr, 2013/05/23 12:38:11]

We are not statically compiling against SHGetKnownFolderPath. We use
delay-loading of Shell32.dll (aka link it dynamically). See the project file.

On 2013/05/23 12:31:20, Felix H. Dahlke wrote:

+    WCHAR* pathBuffer;
+    if (FAILED(SHGetKnownFolderPath(FOLDERID_LocalAppDataLow, 0, 0, &pathBuffer)))
       throw std::runtime_error("Unable to find app data directory");
-    wcscpy_s(appDataPath, dataPath);

[Wladimir Palant, 2013/05/16 11:52:16]

Why do we need two copy operations here? Converting to std::wstring immediately
without any buffers makes more sense.

[Felix Dahlke, 2013/05/17 09:11:36]

I think that's because SHGetKnownFolderPath wants a pointer to a pointer,
setting it to memory it allocates for the path. Further below, however, we
assume it's on the stack.

[Wladimir Palant, 2013/05/17 10:35:02]

Then maybe get rid of that assumption? :)

Declare std::wstring appDataPath above and do appDataPath.assign(dataPath) here.
And move the wchar_t buffer into the else branch since we only need it there.

Please note that SHGetKnownFolderPath() no longer guarantees that the path will
fit into our buffer - it can be longer than MAX_PATH. So better not assume that
it will.

[Felix Dahlke, 2013/05/23 12:31:20]

Done.

-    CoTaskMemFree(dataPath);
+    appDataPath.assign(pathBuffer);
+    CoTaskMemFree(pathBuffer);
   }
   else
   { 
-    if (!SHGetSpecialFolderPath(0, appDataPath, CSIDL_LOCAL_APPDATA, true))
+    std::auto_ptr<wchar_t> pathBuffer(new wchar_t[MAX_PATH]); 
+    if (!SHGetSpecialFolderPath(0, pathBuffer.get(), CSIDL_LOCAL_APPDATA, true))
       throw std::runtime_error("Unable to find app data directory");
-  }
-  return std::wstring(appDataPath) + L"\\AdblockPlus";
+    appDataPath.assign(pathBuffer.get());
+  }
+  return appDataPath + L"\\AdblockPlus";
 }
 
 std::auto_ptr<AdblockPlus::FilterEngine> CreateFilterEngine()
 {
   // TODO: Pass appInfo in, which should be sent by the client
   AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::New();
-  std::string dataPath = ToString(GetAppDataPath());
+  std::string dataPath = ToUtf8String(GetAppDataPath());
   dynamic_cast<AdblockPlus::DefaultFileSystem*>(jsEngine->GetFileSystem().get())->SetBasePath(dataPath);
   std::auto_ptr<AdblockPlus::FilterEngine> filterEngine(new AdblockPlus::FilterEngine(jsEngine));
-  int timeout = 5000;
-  while (!filterEngine->IsInitialized())

[Wladimir Palant, 2013/05/16 11:52:16]

TODO comment here please - we don't really want to poll the filter engine. And
we don't want to make assumptions about how much time it takes to initialize.
E.g. consider the scenario that there is high load on the machine or some disk
I/O that completely occupied the hard disk.

[Felix Dahlke, 2013/05/17 09:11:36]

I presume that's not necessary anymore, now that you apparently made the
FilterEngine block until it's initialised?
(http://codereview.adblockplus.org/10420020/)

[Wladimir Palant, 2013/05/17 10:35:02]

Not pushed yet but - yes, once that review is done we can get rid of the polling
here.

[Felix Dahlke, 2013/05/23 12:31:20]

Done.

-  {
-    const int step = 10;
-    Sleep(step);
-    timeout -= step;
-    if (timeout <= 0)
-      throw std::runtime_error("Timeout while waiting for FilterEngine initialization");
-  }
   std::vector<AdblockPlus::SubscriptionPtr> subscriptions = filterEngine->FetchAvailableSubscriptions();
   // TODO: Select a subscription based on the language, not just the first one.
   //       This should ideally be done in libadblockplus.
   AdblockPlus::SubscriptionPtr subscription = subscriptions[0];
   subscription->AddToList();
   return filterEngine;
 }
 
 HANDLE CreatePipe(const std::wstring& pipeName)
 {
   SECURITY_ATTRIBUTES sa;
   memset(&sa, 0, sizeof(SECURITY_ATTRIBUTES));
   sa.nLength = sizeof(SECURITY_ATTRIBUTES);
 
   // Low mandatory label. See http://msdn.microsoft.com/en-us/library/bb625958.aspx
-  LPCWSTR LOW_INTEGRITY_SDDL_SACL_W = L"S:(ML;;NW;;;LW)";
+  LPCWSTR accessControlEntry = L"S:(ML;;NW;;;LW)";
   PSECURITY_DESCRIPTOR securitydescriptor;
-  ConvertStringSecurityDescriptorToSecurityDescriptor(
-    LOW_INTEGRITY_SDDL_SACL_W, SDDL_REVISION_1, &securitydescriptor, 0);

[Wladimir Palant, 2013/05/16 11:52:16]

You are leaking this security descriptor, it needs to be freed with LocalFree().

[Felix Dahlke, 2013/05/23 12:31:20]

Done.

+  ConvertStringSecurityDescriptorToSecurityDescriptor(accessControlEntry, SDDL_REVISION_1, &securitydescriptor, 0);
 
   sa.lpSecurityDescriptor = securitydescriptor;
   sa.bInheritHandle = TRUE;
 
-  return CreateNamedPipe(pipeName.c_str(), PIPE_ACCESS_DUPLEX, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
-                         PIPE_UNLIMITED_INSTANCES, bufferSize, bufferSize, 0, &sa);
+  HANDLE pipe = CreateNamedPipe(pipeName.c_str(), PIPE_ACCESS_DUPLEX, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
+                                PIPE_UNLIMITED_INSTANCES, bufferSize, bufferSize, 0, &sa);
+  LocalFree(securitydescriptor);
+  return pipe;
 }
 
 int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int)

[Wladimir Palant, 2013/05/16 11:52:16]

Second HINSTANCE parameter is hPrevInstance - you probably want to exit
immediately if it is set (client got impatient).

[Felix Dahlke, 2013/05/23 12:31:20]

Seems to be always 0 in Win32:
http://blogs.msdn.com/b/oldnewthing/archive/2004/06/15/156022.aspx

[Wladimir Palant, 2013/05/23 14:25:01]

Right, a bit of outdated knowledge on my end :-(
Finding an already running application instance is still necessary, just more
complicated - named mutexes should do. Note that the name of this mutex should
also be user-specific.

[Felix Dahlke, 2013/05/23 19:10:44]

How about we just try to create the pipe initially and return immediately if it
already exists? We'll want to do the CreatePipe earlier anyway I think.

[Wladimir Palant, 2013/05/23 20:10:15]

Yes, that's something I thought about as well. This should still make sure there
aren't any race conditions.

 {
   filterEngine = CreateFilterEngine();
-  filterEngineMutex = CreateMutex(0, false, 0);
 
   for (;;)
   {
     HANDLE pipe = CreatePipe(pipeName);
     if (pipe == INVALID_HANDLE_VALUE)
     {
       LogLastError("CreateNamedPipe failed");
       return 1;
     }
 
     if (!ConnectNamedPipe(pipe, 0))
     {
       LogLastError("Client failed to connect");
       CloseHandle(pipe);
       continue;
     }
 
     // TODO: Count established connections, kill the engine when none are left
 
     AutoHandle thread(CreateThread(0, 0, ClientThread, static_cast<LPVOID>(pipe), 0, 0));
     if (!thread.get())
     {
       LogLastError("CreateThread failed");
       return 1;
     }
   }
 
   return 0;
 }

[Wladimir Palant, 2013/05/16 11:52:16]

IMHO, ReadMessage(), WriteMessage(), MarshalStrings() and UnmarshalStrings()
should be shared between the engine and the client - you need a source file
shared by the two projects.

[Felix Dahlke, 2013/05/17 09:11:36]

Same here, there's a TODO for that :)