Issue #1173 - Protect more entry points with try-catch blocks

src/plugin/PluginClass.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 953 matching lines @@
     if (it != s_threadInstances.end())
     {
       tab = it->second->m_tab;
     }
   }
   s_criticalSectionLocal.Unlock();
 
   return tab;
 }
 
-
+// Entry point
 STDMETHODIMP CPluginClass::QueryStatus(const GUID* pguidCmdGroup, ULONG cCmds, OLECMD prgCmds[], OLECMDTEXT* pCmdText)
 {
-  if (cCmds == 0) return E_INVALIDARG;
-  if (prgCmds == 0) return E_POINTER;
+  try
+  {
+    if (cCmds == 0) return E_INVALIDARG;
+    if (prgCmds == 0) return E_POINTER;
 
-  prgCmds[0].cmdf = OLECMDF_ENABLED;
-
+    prgCmds[0].cmdf = OLECMDF_ENABLED;
+  }
+  catch (...)
+  {
+    return E_FAIL;
+  }
   return S_OK;
 }
 
 HMENU CPluginClass::CreatePluginMenu(const std::wstring& url)
 {
   DEBUG_GENERAL("CreatePluginMenu");
   HINSTANCE hInstance = _AtlBaseModule.GetModuleInstance();
 
   HMENU hMenu = ::LoadMenu(hInstance, MAKEINTRESOURCE(IDR_MENU1));
 
@@ skipping 181 matching lines @@
   ctext = dictionary->Lookup("menu", "menu-settings");
   fmii.fMask  = MIIM_STATE | MIIM_STRING;
   fmii.fState = MFS_ENABLED;
   fmii.dwTypeData = const_cast<LPWSTR>(ctext.c_str());
   fmii.cch = static_cast<UINT>(ctext.size());
   ::SetMenuItemInfoW(hMenu, ID_MENU_SETTINGS, FALSE, &fmii);
 
   return true;
 }
 
-
+// Entry point
 STDMETHODIMP CPluginClass::Exec(const GUID*, DWORD nCmdID, DWORD, VARIANTARG*, VARIANTARG*)
 {
-  HWND hBrowserWnd = GetBrowserHWND();
-  if (!hBrowserWnd)
+  try
   {
-    return E_FAIL;
-  }
+    HWND hBrowserWnd = GetBrowserHWND();
+    if (!hBrowserWnd)
+    {
+      return E_FAIL;
+    }
 
-  // Create menu
-  HMENU hMenu = CreatePluginMenu(m_tab->GetDocumentUrl());
-  if (!hMenu)
-  {
-    return E_FAIL;
-  }
+    // Create menu
+    HMENU hMenu = CreatePluginMenu(m_tab->GetDocumentUrl());
+    if (!hMenu)
+    {
+      return E_FAIL;
+    }
 
-  // Check if button in toolbar was pressed
-  int nIDCommand = -1;
-  BOOL bRightAlign = FALSE;
+    // Check if button in toolbar was pressed
+    int nIDCommand = -1;
+    BOOL bRightAlign = FALSE;
 
-  POINT pt;
-  GetCursorPos(&pt);
+    POINT pt;
+    GetCursorPos(&pt);
 
-  HWND hWndToolBar = ::WindowFromPoint(pt);
+    HWND hWndToolBar = ::WindowFromPoint(pt);
 
-  DWORD nProcessId;
-  ::GetWindowThreadProcessId(hWndToolBar, &nProcessId);
+    DWORD nProcessId;
+    ::GetWindowThreadProcessId(hWndToolBar, &nProcessId);
 
-  if (hWndToolBar && ::GetCurrentProcessId() == nProcessId)
-  {
-    ::ScreenToClient(hWndToolBar, &pt);
-    int nButton = (int)::SendMessage(hWndToolBar, TB_HITTEST, 0, (LPARAM)&pt);
+    if (hWndToolBar && ::GetCurrentProcessId() == nProcessId)
+    {
+      ::ScreenToClient(hWndToolBar, &pt);
+      int nButton = (int)::SendMessage(hWndToolBar, TB_HITTEST, 0, (LPARAM)&pt);
 
-    if (nButton > 0)
-    {
-      TBBUTTON pTBBtn = {};
+      if (nButton > 0)
+      {
+        TBBUTTON pTBBtn = {};
 
-      if (SendMessage(hWndToolBar, TB_GETBUTTON, nButton, (LPARAM)&pTBBtn))
-      {
-        RECT rcButton;
-        nIDCommand = pTBBtn.idCommand;
+        if (SendMessage(hWndToolBar, TB_GETBUTTON, nButton, (LPARAM)&pTBBtn))
+        {
+          RECT rcButton;
+          nIDCommand = pTBBtn.idCommand;
 
-        if (SendMessage(hWndToolBar, TB_GETRECT, nIDCommand, (LPARAM)&rcButton))
-        {
-          pt.x = rcButton.left;
-          pt.y = rcButton.bottom;
-          ClientToScreen(hWndToolBar, &pt);
-
-          RECT rcWorkArea;
-          SystemParametersInfo(SPI_GETWORKAREA, 0, (LPVOID)&rcWorkArea, 0);
-          if (rcWorkArea.right - pt.x < 150)
+          if (SendMessage(hWndToolBar, TB_GETRECT, nIDCommand, (LPARAM)&rcButton))
           {
-            bRightAlign = TRUE;
-            pt.x = rcButton.right;
+            pt.x = rcButton.left;
             pt.y = rcButton.bottom;
             ClientToScreen(hWndToolBar, &pt);
+
+            RECT rcWorkArea;
+            SystemParametersInfo(SPI_GETWORKAREA, 0, (LPVOID)&rcWorkArea, 0);
+            if (rcWorkArea.right - pt.x < 150)
+            {
+              bRightAlign = TRUE;
+              pt.x = rcButton.right;
+              pt.y = rcButton.bottom;
+              ClientToScreen(hWndToolBar, &pt);
+            }
           }
         }
       }
+      else
+      {
+        GetCursorPos(&pt);
+      }
+    }
+
+    // Display menu
+    UINT nFlags = 0;
+    if (bRightAlign)
+    {
+      nFlags |= TPM_RIGHTALIGN;
     }
     else
     {
-      GetCursorPos(&pt);
+      nFlags |= TPM_LEFTALIGN;
     }
+
+    DisplayPluginMenu(hMenu, nIDCommand, pt, nFlags);
   }
-
-  // Display menu
-  UINT nFlags = 0;
-  if (bRightAlign)
+  catch (...)
   {
-    nFlags |= TPM_RIGHTALIGN;
+    return E_FAIL; //

[Oleksandr, 2016/01/04 10:46:13]

Nit: there was a comment intended here?

[Eric, 2016/01/04 16:44:24]

Yes. Added.

   }
-  else
-  {
-    nFlags |= TPM_LEFTALIGN;
-  }
-
-  DisplayPluginMenu(hMenu, nIDCommand, pt, nFlags);
 
   return S_OK;
 }
 
-/////////////////////////////////////////////////////////////////////////////
-// Window procedures
-
+// Entry point
 LRESULT CALLBACK CPluginClass::NewStatusProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
 {
-  // Find tab
-  CPluginClass *pClass = FindInstance(hWnd);
-  if (!pClass)
+  try
   {
-    return DefWindowProc(hWnd, message, wParam, lParam);
-  }
+    // Find tab
+    CPluginClass *pClass = FindInstance(hWnd);
+    if (!pClass)
+    {
+      return DefWindowProc(hWnd, message, wParam, lParam);

[Eric, 2016/01/04 16:44:24]

Race condition here.

+    }
 
-  // Process message
-  switch (message)
-  {
-  case SB_SIMPLE:
+    // Process message
+    switch (message)
+    {
+    case SB_SIMPLE:
     {
       ShowWindow(pClass->m_hPaneWnd, !wParam);
       break;
     }
 
-  case WM_SYSCOLORCHANGE:
+    case WM_SYSCOLORCHANGE:
     {
       pClass->UpdateTheme();
       break;
     }
 
-  case SB_SETPARTS:
+    case SB_SETPARTS:
     {
       if (!lParam || !wParam || wParam > 30 || !IsWindow(pClass->m_hPaneWnd))
       {
         return CallWindowProc(pClass->m_pWndProcStatus, hWnd, message, wParam, lParam);
       }
 
       WPARAM nParts = wParam;
       if (STATUSBAR_PANE_NUMBER >= nParts)
       {
         return CallWindowProc(pClass->m_pWndProcStatus, hWnd, message, wParam, lParam);
       }
 
-      HLOCAL hLocal = LocalAlloc(LHND, sizeof(int) * (nParts+1));
+      HLOCAL hLocal = LocalAlloc(LHND, sizeof(int) * (nParts + 1));
       LPINT lpParts = (LPINT)LocalLock(hLocal);
       memcpy(lpParts, (void*)lParam, wParam*sizeof(int));
 
       for (unsigned i = 0; i < STATUSBAR_PANE_NUMBER; i++)
       {
         lpParts[i] -= pClass->m_nPaneWidth;
       }
       LRESULT hRet = CallWindowProc(pClass->m_pWndProcStatus, hWnd, message, wParam, (LPARAM)lpParts);
 
       AdblockPlus::Rectangle rcPane;
       ::SendMessage(hWnd, SB_GETRECT, STATUSBAR_PANE_NUMBER, (LPARAM)&rcPane);
 
       AdblockPlus::Rectangle rcClient;
       ::GetClientRect(hWnd, &rcClient);
 
       ::MoveWindow(
         pClass->m_hPaneWnd,
         lpParts[STATUSBAR_PANE_NUMBER] - pClass->m_nPaneWidth,
         0,
         pClass->m_nPaneWidth,
         rcClient.Height(),
         TRUE);
 
       ::LocalFree(hLocal);
 
 
       return hRet;
     }
 
-  default:
-    break;
-  }
+    default:
+      break;
+    }
 
-  LRESULT result = CallWindowProc(pClass->m_pWndProcStatus, hWnd, message, wParam, lParam);
+    LRESULT result = CallWindowProc(pClass->m_pWndProcStatus, hWnd, message, wParam, lParam);
 
 
-  return result;
-
+    return result;
+  }
+  catch (...)
+  {
+    return E_FAIL;

[Oleksandr, 2016/01/04 10:46:13]

I think we should call: return DefWindowProc(hWnd, message, wParam, lParam) here
instead. This is not a COM method so returning E_FAIL doesn't seem right.

[sergei, 2016/01/04 11:27:36]

It's better to use CallWindowProc instead of DefWindowProc here because we are
subclassing the window.

[Eric, 2016/01/04 16:44:24]

Sergei's right.

This observation also points out a defect on line 1287.

+  }
 }
 
 
 HICON CPluginClass::GetStatusBarIcon(const std::wstring& url)
 {
   // use the disable icon as defualt, if the client doesn't exists
   HICON hIcon = GetIcon(ICON_PLUGIN_DEACTIVATED);
 
   CPluginTab* tab = GetTab(::GetCurrentThreadId());
   if (tab)
   {
     CPluginClient* client = CPluginClient::GetInstance();
     if (CPluginSettings::GetInstance()->IsPluginEnabled())
     {
       if (client->IsWhitelistedUrl(url))
       {
         hIcon = GetIcon(ICON_PLUGIN_DISABLED);
       }
       else
       {
         CPluginSettings* settings = CPluginSettings::GetInstance();
         hIcon = GetIcon(ICON_PLUGIN_ENABLED);
       }
     }
   }
   return hIcon;
 }
 
-
+// Entry point
 LRESULT CALLBACK CPluginClass::PaneWindowProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
 {
-  // Find tab
-  CPluginClass *pClass = FindInstance(GetParent(hWnd));
-  if (!pClass)
+  try
   {
-    return ::DefWindowProc(hWnd, message, wParam, lParam);
-  }
+    // Find tab
+    CPluginClass *pClass = FindInstance(GetParent(hWnd));
+    if (!pClass)
+    {
+      return ::DefWindowProc(hWnd, message, wParam, lParam);
+    }
 
-  // Process message
-  switch (message)
-  {
+    // Process message
+    switch (message)
+    {
 
-  case WM_SETCURSOR:
+    case WM_SETCURSOR:
     {
       ::SetCursor(::LoadCursor(NULL, IDC_ARROW));
       return TRUE;
     }
-  case WM_PAINT:
+    case WM_PAINT:
     {
       PAINTSTRUCT ps;
       HDC hDC = ::BeginPaint(hWnd, &ps);
 
       AdblockPlus::Rectangle rcClient;
       ::GetClientRect(hWnd, &rcClient);
 
       int nDrawEdge = 0;
 
       // Old Windows background drawing
@@ skipping 47 matching lines @@
         {
           //Get the RECT for the leftmost pane (the status text pane)
           RECT rect;
           BOOL rectRes = ::SendMessage(pClass->m_hStatusBarWnd, SB_GETRECT, 0, (LPARAM)&rect);
           ::DrawIconEx(hDC, 0, rect.bottom - rect.top - iconHeight, hIcon, iconWidth, iconHeight, NULL, NULL, DI_NORMAL);
           offx += iconWidth;
         }
 #ifdef _DEBUG
         // Display version
         HFONT hFont = (HFONT)::SendMessage(pClass->m_hStatusBarWnd, WM_GETFONT, 0, 0);
-        HGDIOBJ hOldFont = ::SelectObject(hDC,hFont);
+        HGDIOBJ hOldFont = ::SelectObject(hDC, hFont);
 
         AdblockPlus::Rectangle rcText = rcClient;
         rcText.left += offx;
         ::SetBkMode(hDC, TRANSPARENT);
-        ::DrawTextW(hDC, IEPLUGIN_VERSION, -1, &rcText, DT_WORD_ELLIPSIS|DT_LEFT|DT_SINGLELINE|DT_VCENTER);
+        ::DrawTextW(hDC, IEPLUGIN_VERSION, -1, &rcText, DT_WORD_ELLIPSIS | DT_LEFT | DT_SINGLELINE | DT_VCENTER);
 
         ::SelectObject(hDC, hOldFont);
 #endif // _DEBUG
       }
 
       // Done!
       EndPaint(hWnd, &ps);
 
       return 0;
     }
 
-  case WM_LBUTTONUP:
-  case WM_RBUTTONUP:
+    case WM_LBUTTONUP:
+    case WM_RBUTTONUP:
     {
       std::wstring url = pClass->GetBrowserUrl();
       if (url != pClass->GetTab()->GetDocumentUrl())
       {
         pClass->GetTab()->SetDocumentUrl(url);
       }
 
       // Create menu
       HMENU hMenu = pClass->CreatePluginMenu(url);
       if (!hMenu)
       {
         return 0;
       }
 
       // Display menu
       POINT pt;
       ::GetCursorPos(&pt);
 
       RECT rc;
       ::GetWindowRect(hWnd, &rc);
 
       if (rc.left >= 0 && rc.top >= 0)
       {
         pt.x = rc.left;
         pt.y = rc.top;
       }
 
-      pClass->DisplayPluginMenu(hMenu, -1, pt, TPM_LEFTALIGN|TPM_BOTTOMALIGN);
+      pClass->DisplayPluginMenu(hMenu, -1, pt, TPM_LEFTALIGN | TPM_BOTTOMALIGN);
     }
     break;
-  case WM_DESTROY:
-    break;
-  case SC_CLOSE:
-    break;
+    case WM_DESTROY:
+      break;
+    case SC_CLOSE:
+      break;
 
-  case WM_UPDATEUISTATE:
+    case WM_UPDATEUISTATE:
     {
       CPluginTab* tab = GetTab(::GetCurrentThreadId());
       if (tab)
       {
         tab->OnActivate();
         RECT rect;
         GetWindowRect(pClass->m_hPaneWnd, &rect);
-        pClass->notificationMessage.Move(rect.left + (rect.right - rect.left) / 2, rect.top + (rect.bottom - rect.top) / 2); 
+        pClass->notificationMessage.Move(rect.left + (rect.right - rect.left) / 2, rect.top + (rect.bottom - rect.top) / 2);
       }
       if (LOWORD(wParam) == UIS_CLEAR)
       {
         pClass->notificationMessage.Hide();
       }
     }
     break;
-  case WM_WINDOWPOSCHANGING:
+    case WM_WINDOWPOSCHANGING:
     {
       RECT rect;
       GetWindowRect(pClass->m_hPaneWnd, &rect);
       if (pClass->notificationMessage.IsVisible())
       {
-        pClass->notificationMessage.Move(rect.left + (rect.right - rect.left) / 2, rect.top + (rect.bottom - rect.top) / 2); 
+        pClass->notificationMessage.Move(rect.left + (rect.right - rect.left) / 2, rect.top + (rect.bottom - rect.top) / 2);
       }
     }
     break;
-  case WM_WINDOWPOSCHANGED:
+    case WM_WINDOWPOSCHANGED:
     {
       WINDOWPOS* wndPos = reinterpret_cast<WINDOWPOS*>(lParam);
       if (wndPos->flags & SWP_HIDEWINDOW)
       {
         pClass->notificationMessage.Hide();
       }
     }
     break;
-  case WM_ALREADY_UP_TO_DATE:
+    case WM_ALREADY_UP_TO_DATE:
     {
       Dictionary* dictionary = Dictionary::GetInstance();
       std::wstring upToDateText = dictionary->Lookup("updater", "update-already-up-to-date-text");
       std::wstring upToDateTitle = dictionary->Lookup("updater", "update-already-up-to-date-title");
       pClass->notificationMessage.SetTextAndIcon(upToDateText, upToDateTitle, TTI_INFO);
     }
     break;
-  case WM_UPDATE_CHECK_ERROR:
+    case WM_UPDATE_CHECK_ERROR:
     {
       Dictionary* dictionary = Dictionary::GetInstance();
       std::wstring errorText = dictionary->Lookup("updater", "update-error-text");
       std::wstring errorTitle = dictionary->Lookup("updater", "update-error-title");
       pClass->notificationMessage.SetTextAndIcon(errorText, errorText, TTI_ERROR);
     }
     break;
-  case WM_DOWNLOADING_UPDATE:
+    case WM_DOWNLOADING_UPDATE:
     {
       Dictionary* dictionary = Dictionary::GetInstance();
       std::wstring downloadingText = dictionary->Lookup("updater", "downloading-update-text");
       std::wstring downloadingTitle = dictionary->Lookup("updater", "downloading-update-title");
       pClass->notificationMessage.SetTextAndIcon(downloadingText, downloadingTitle, TTI_INFO);
     }
     break;
+    }
+
+    return DefWindowProc(hWnd, message, wParam, lParam);
   }
-
-  return DefWindowProc(hWnd, message, wParam, lParam);
+  catch (...)
+  {
+    return E_FAIL;

[Oleksandr, 2016/01/04 10:46:13]

Same here. I think we should do 'return DefWindowProc(hWnd, message, wParam,
lParam);' instead.

[Eric, 2016/01/04 16:44:24]

Done.

Replace with a comment. Moved call to 'DefWindowProc' to the end, outside the
'try' statement, since it doesn't throw.

+  }
 }
 
 
 void CPluginClass::UpdateStatusBar()
 {
   DEBUG_GENERAL("*** Updating statusbar")
   if (m_hPaneWnd == NULL)
   {
     CreateStatusBarPane();
   }
@@ skipping 48 matching lines @@
   s_criticalSectionLocal.Unlock();
 
   return icon;
 }
 
 ATOM CPluginClass::GetAtomPaneClass()
 {
   return s_atomPaneClass;
 }