Enhanced Protected Mode support

src/plugin/AdblockPlusClient.cpp

 #include "PluginStdAfx.h"
-
-#include <Windows.h>
-#include <Sddl.h>

[Felix Dahlke, 2013/09/16 16:30:12]

Shouldn't these two includes go into PluginStdAfx?

-
-
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #include "PluginFilter.h"
 #include "PluginClientFactory.h"
 #include "PluginMutex.h"
 #include "PluginClass.h"
 
 #include "AdblockPlusClient.h"
 
 #include "../shared/Utils.h"
 
 namespace
 {
   void SpawnAdblockPlusEngine()
   {
     std::wstring engineExecutablePath = GetDllDir() + L"AdblockPlusEngine.exe";
     CString params = L"AdblockPlusEngine.exe " + CPluginSystem::GetInstance()->GetBrowserLanguage();
 
     STARTUPINFO startupInfo = {};
     PROCESS_INFORMATION processInformation = {};
 
     HANDLE token;
     OpenProcessToken(GetCurrentProcess(), TOKEN_DUPLICATE | TOKEN_ADJUST_DEFAULT | TOKEN_QUERY | TOKEN_ASSIGN_PRIMARY, &token);
 
     TOKEN_APPCONTAINER_INFORMATION *acSid = NULL;

[Wladimir Palant, 2013/09/17 07:53:48]

Nit: that variable name also uses the Hungarian notation.

-    DWORD dwLength = 0;

[Wladimir Palant, 2013/09/16 13:45:07]

Nit: Since when are we using Hungarian notation? I'd suggest dropping the prefix
in the variable names.

+    DWORD length = 0;
 
     // Get AppContainer SID
-    if (!GetTokenInformation(token, TokenAppContainerSid, (LPVOID) acSid, 0, &dwLength) && GetLastError() == ERROR_INSUFFICIENT_BUFFER)

[Wladimir Palant, 2013/09/16 13:45:07]

Nit: I think that the explicit cast to LPVOID here and below is unnecessary.

-    {
-        acSid = (TOKEN_APPCONTAINER_INFORMATION*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength);

[Wladimir Palant, 2013/09/16 13:45:07]

Why are we using HeapAlloc() rather than "new" here? The latter can be used with
a smart pointer which allows ignoring the cleanup. Speaking of cleanup, are we
leaking that pointer?

Nit: use static_cast for the type conversion here?

[Felix Dahlke, 2013/09/16 16:30:12]

Nit: Space before HeapAlloc?

[Oleksandr, 2013/09/17 03:11:37]

"new" isn't good here since in the specific example the required number of bytes
to allocate is 48. And sizeof(TOKEN_APPCONTAIER_INFORMATION) is not 48. Some
sort of trickery would still have to be made even if auto_ptr was used.

[Wladimir Palant, 2013/09/17 07:53:48]

What I actually meant:

std::unique_ptr<char[]> sid(new char[length]);

You can later cast the pointer to TOKEN_APPCONTAINER_INFORMATION when you need
it. Note that unique_ptr will deal with arrays correctly, unlike auto_ptr.

+    if (!GetTokenInformation(token, TokenAppContainerSid, acSid, 0, &length) && GetLastError() == ERROR_INSUFFICIENT_BUFFER)
+    {
+        acSid = (TOKEN_APPCONTAINER_INFORMATION*) HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, length);
         if (acSid != NULL)

[Felix Dahlke, 2013/09/16 16:30:12]

What if the allocation failed? Isn't that worth an exception?

         {
-          GetTokenInformation(token, TokenAppContainerSid, (LPVOID) acSid, dwLength, &dwLength);
+          GetTokenInformation(token, TokenAppContainerSid, acSid, length, &length);
+        }
+        else
+        {
+          throw std::runtime_error("Out of memory");
         }
     }
 
     BOOL createProcRes = 0;
     // Running inside AppContainer?
-    if ((acSid != NULL) && (acSid->TokenAppContainer != NULL))

[Wladimir Palant, 2013/09/16 13:45:07]

Nit: the extra parentheses are unnecessary.

+    if (acSid != NULL && acSid->TokenAppContainer != NULL)
     {
       // Launch with default security. Registry entry will eat the user prompt
       // See http://msdn.microsoft.com/en-us/library/bb250462(v=vs.85).aspx#wpm_elebp
       LPWSTR stringSid;
       ConvertSidToStringSidW(acSid->TokenAppContainer, &stringSid);
       params.Append(L" ");
       params.Append(stringSid);
       LocalFree(stringSid);
-      createProcRes = CreateProcess(engineExecutablePath.c_str(), params.GetBuffer(params.GetLength() + 1),
+      createProcRes = CreateProcessW(engineExecutablePath.c_str(), params.GetBuffer(params.GetLength() + 1),
                               0, 0, false, 0, 0, 0, (STARTUPINFOW*)&startupInfo, &processInformation);
     }
     else
     {
       // Launch with the same security token (Low Integrity) explicitly
       HANDLE newToken;
       DuplicateTokenEx(token, 0, 0, SecurityImpersonation, TokenPrimary, &newToken);
 
       createProcRes = CreateProcessAsUser(newToken, engineExecutablePath.c_str(), params.GetBuffer(params.GetLength() + 1),

[Wladimir Palant, 2013/09/17 07:53:48]

Nit: This should be changed into CreateProcessAsUserW as well.

                               0, 0, false, 0, 0, 0, (STARTUPINFOW*)&startupInfo, &processInformation);
     }
 
     if (!createProcRes)
     {
       throw std::runtime_error("Failed to start Adblock Plus Engine");
     }
 
     CloseHandle(processInformation.hProcess);
     CloseHandle(processInformation.hThread);
@@ skipping 16 matching lines @@
         try
         {
           return new Communication::Pipe(Communication::pipeName, Communication::Pipe::MODE_CONNECT);
         }
         catch (Communication::PipeConnectionError e)
         {
         }
       }
       throw std::runtime_error("Unable to open Adblock Plus Engine pipe");
     }
-    catch(...)

[Wladimir Palant, 2013/09/16 13:45:07]

I don't really like seeing "catch all", what kind of error are you expecting
here that we react to by spawing the engine?

-    {
-      SpawnAdblockPlusEngine();
-    }
   }
 
   std::vector<std::wstring> ReadStrings(Communication::InputBuffer& message)
   {
     int32_t count;
     message >> count;
 
     std::vector<std::wstring> result;
     for (int32_t i = 0; i < count; i++)
     {
@@ skipping 359 matching lines @@
     DEBUG_GENERAL((L"GetPref: " + name + L" end").c_str());
     return value;
   }
   else
   {
     DEBUG_GENERAL((L"GetPref: " + name + L" end").c_str());
     return defaultValue;
   }
 }
 
-void CAdblockPlusClient::CheckForUpdates(HWND callbackWindow)
-{
-  Communication::OutputBuffer request;
-  request << Communication::PROC_CHECK_FOR_UPDATES << (int32_t&)callbackWindow;
-  CallEngine(request);
+void CAdblockPlusClient::CheckForUpdates()
+{
+  CallEngine(Communication::PROC_CHECK_FOR_UPDATES);
 }
 
 std::wstring CAdblockPlusClient::GetDocumentationLink()
 {
   DEBUG_GENERAL("GetDocumentationLink");
   Communication::InputBuffer response;
   if (!CallEngine(Communication::PROC_GET_DOCUMENTATION_LINK, response)) 
     return L"";
   std::wstring docLink;
   response >> docLink;
   return docLink;
 }
 
 bool CAdblockPlusClient::TogglePluginEnabled()
 {
   DEBUG_GENERAL("TogglePluginEnabled");
   Communication::InputBuffer response;
   if (!CallEngine(Communication::PROC_TOGGLE_PLUGIN_ENABLED, response)) 
     return false;
   bool currentEnabledState;
   response >> currentEnabledState;
   return currentEnabledState;
 }