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

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -971,14 +971,20 @@
   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;
 }
 
@@ -1180,116 +1186,123 @@
   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))
       {
@@ -1302,7 +1315,7 @@
         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));
 
@@ -1332,15 +1345,19 @@
       return hRet;
     }
 
-  default:
-    break;
+    default:
+      break;
+    }
+
+    LRESULT result = CallWindowProc(pClass->m_pWndProcStatus, hWnd, message, wParam, lParam);
+
+
+    return result;
   }
-
-  LRESULT result = CallWindowProc(pClass->m_pWndProcStatus, hWnd, message, wParam, lParam);
-
-
-  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.

+  }
 }
 
 
@@ -1369,26 +1386,28 @@
   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);
@@ -1456,12 +1475,12 @@
 #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
@@ -1473,8 +1492,8 @@
       return 0;
     }
 
-  case WM_LBUTTONUP:
-  case WM_RBUTTONUP:
+    case WM_LBUTTONUP:
+    case WM_RBUTTONUP:
     {
       std::wstring url = pClass->GetBrowserUrl();
       if (url != pClass->GetTab()->GetDocumentUrl())
@@ -1502,15 +1521,15 @@
         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)
@@ -1518,7 +1537,7 @@
         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)
       {
@@ -1526,17 +1545,17 @@
       }
     }
     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)
@@ -1545,7 +1564,7 @@
       }
     }
     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");
@@ -1553,7 +1572,7 @@
       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");
@@ -1561,7 +1580,7 @@
       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");
@@ -1569,9 +1588,14 @@
       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.

+  }
 }