Issue 1283 - wrong usage of memset, fix sizeof, make proper initializing

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -16,6 +16,7 @@
 #include "../shared/Utils.h"
 #include "../shared/Dictionary.h"
 #include <thread>
+#include <array>
 
 #ifdef DEBUG_HIDE_EL
 DWORD profileTime = 0;
@@ -421,7 +422,7 @@
   HKEY pHkeySub;
   RegOpenCurrentUser(KEY_QUERY_VALUE, &pHkey);
   DWORD trueth = 1;

[Oleksandr, 2014/10/06 20:19:32]

That typo really jumps at me with this change. Mind changing it to "truth" in
this patch?

-  DWORD truethSize = sizeof(DWORD);
+  DWORD truethSize = sizeof(trueth);
   RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\Main", &pHkeySub);
   LONG res = RegQueryValueEx(pHkeySub, L"StatusBarWeb", NULL, NULL, (BYTE*)&trueth, &truethSize);
   RegCloseKey(pHkey);
@@ -485,10 +486,10 @@
         {
           DWORD trueth = 1;
           regRes = RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\MINIE", &pHkeySub);
-          regRes = RegSetValueEx(pHkeySub, L"ShowStatusBar", 0, REG_DWORD, (BYTE*)&trueth, sizeof(DWORD));
+          regRes = RegSetValueEx(pHkeySub, L"ShowStatusBar", 0, REG_DWORD, (BYTE*)&trueth, sizeof(trueth));
           regRes = RegCloseKey(pHkeySub);
           regRes = RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\Main", &pHkeySub);
-          regRes = RegSetValueEx(pHkeySub, L"StatusBarWeb", 0, REG_DWORD, (BYTE*)&trueth, sizeof(DWORD));
+          regRes = RegSetValueEx(pHkeySub, L"StatusBarWeb", 0, REG_DWORD, (BYTE*)&trueth, sizeof(trueth));
           regRes = RegCloseKey(pHkeySub);
           hr = browser->put_StatusBar(TRUE);
           if (FAILED(hr))
@@ -813,7 +814,7 @@
   {
     WNDCLASSEX wcex;
 
-    wcex.cbSize = sizeof(WNDCLASSEX);
+    wcex.cbSize = sizeof(wcex);
     wcex.style = 0;
     wcex.lpfnWndProc = (WNDPROC)PaneWindowProc;
     wcex.cbClsExtra = 0;
@@ -889,7 +890,7 @@
 
   CPluginClient* client = CPluginClient::GetInstance();
 
-  wchar_t szClassName[MAX_PATH];
+  std::array<wchar_t, MAX_PATH> className;
   // Get browser window and url
   HWND hBrowserWnd = GetBrowserHWND();
   if (!hBrowserWnd)
@@ -907,14 +908,14 @@
   HWND uniqueNewTab = NULL;
   while (hTabWnd)
   {
-    memset(szClassName, 0, MAX_PATH);

[Eric, 2014/09/26 18:07:23]

The title of the review says "wrong usage of memset". Is this what you meant?
It's not wrong so much as unnecessary.

-    GetClassName(hTabWnd, szClassName, MAX_PATH);
+    className.fill(L'\0');

[Eric, 2014/09/26 18:07:23]

Are these calls to 'fill' really necessary?

The only case I can think of is when GetClassName writes nothing to the buffer,
but in that case, it would be better to handle the return value as an error and
not ignore it. Or if we don't want to do that, just writing an initial
terminating null would be sufficient. But it's probably better to handle the
error correctly.

+    GetClassName(hTabWnd, className.data(), className.size());

[Eric, 2014/09/26 18:07:23]

As long as we're making a modification, we might as well change to
'GetClassNameW', which we've been slowly doing elsewhere.

[Oleksandr, 2014/10/06 20:19:32]

+1
On 2014/09/26 18:07:23, Eric wrote:

[sergei, 2014/10/08 15:06:52]

Despite it's not a real performance trouble, I've fixed it.

 
-    if (wcscmp(szClassName, L"TabWindowClass") == 0 || wcscmp(szClassName,L"Frame Tab") == 0)
+    if (wcscmp(className.data(), L"TabWindowClass") == 0 || wcscmp(className.data(), L"Frame Tab") == 0)
     {
       // IE8 support
       HWND hTabWnd2 = hTabWnd;
-      if (wcscmp(szClassName,L"Frame Tab") == 0)
+      if (wcscmp(className.data(), L"Frame Tab") == 0)
       {
         hTabWnd2 = ::FindWindowEx(hTabWnd2, NULL, L"TabWindowClass", NULL);
       }
@@ -961,10 +962,10 @@
   HWND hWnd = ::GetWindow(hBrowserWnd, GW_CHILD);
   while (hWnd)
   {
-    memset(szClassName, 0, MAX_PATH);
-    ::GetClassName(hWnd, szClassName, MAX_PATH);
+    className.fill(L'\0');
+    ::GetClassName(hWnd, className.data(), className.size());
 
-    if (wcscmp(szClassName,L"msctls_statusbar32") == 0)
+    if (wcscmp(className.data(), L"msctls_statusbar32") == 0)
     {
       hWndStatusBar = hWnd;
       break;
@@ -1161,116 +1162,6 @@
 
   return hMenuTrackPopup;
 }
-BOOL CreateLowProcess(WCHAR* wszProcessName, WCHAR* cmdLine)
-{
-
-  BOOL                  fRet;
-  HANDLE                hToken        = NULL;
-  HANDLE                hNewToken     = NULL;
-  PSID                  pIntegritySid = NULL;
-  TOKEN_MANDATORY_LABEL TIL           = {0};
-  PROCESS_INFORMATION   ProcInfo      = {0};
-  STARTUPINFO           StartupInfo   = {0};
-
-
-
-  // Low integrity SID
-  WCHAR wszIntegritySid[20] = L"S-1-16-4096";
-
-
-  fRet = OpenProcessToken(GetCurrentProcess(),
-    TOKEN_DUPLICATE |
-    TOKEN_ADJUST_DEFAULT |
-    TOKEN_QUERY |
-    TOKEN_ASSIGN_PRIMARY,
-    &hToken);
-
-  if (!fRet)
-  {
-    goto CleanExit;
-  }
-
-  fRet = DuplicateTokenEx(hToken,
-    0,
-    NULL,
-    SecurityImpersonation,
-    TokenPrimary,
-    &hNewToken);
-
-  if (!fRet)
-  {
-    goto CleanExit;
-  }
-
-  fRet = ConvertStringSidToSid(wszIntegritySid, &pIntegritySid);
-
-  if (!fRet)
-  {
-    goto CleanExit;
-  }
-
-
-  TIL.Label.Attributes = SE_GROUP_INTEGRITY;
-  TIL.Label.Sid        = pIntegritySid;
-
-
-  //
-  // Set the process integrity level
-  //
-
-  fRet = SetTokenInformation(hNewToken,
-    TokenIntegrityLevel,
-    &TIL,
-    sizeof(TOKEN_MANDATORY_LABEL) + GetLengthSid(pIntegritySid));
-
-  if (!fRet)
-  {
-    goto CleanExit;
-  }
-
-  //
-  // Create the new process at Low integrity
-  //
-
-  fRet  = CreateProcessAsUser(hNewToken,
-    wszProcessName,
-    cmdLine,
-    NULL,
-    NULL,
-    FALSE,
-    0,
-    NULL,
-    NULL,
-    &StartupInfo,
-    &ProcInfo);
-
-
-CleanExit:
-
-  if (ProcInfo.hProcess != NULL)
-  {
-    CloseHandle(ProcInfo.hProcess);
-  }
-
-  if (ProcInfo.hThread != NULL)
-  {
-    CloseHandle(ProcInfo.hThread);
-  }
-
-  LocalFree(pIntegritySid);
-
-  if (hNewToken != NULL)
-  {
-    CloseHandle(hNewToken);
-  }
-
-  if (hToken != NULL)
-  {
-    CloseHandle(hToken);
-  }
-
-  return fRet;
-}
 
 void CPluginClass::DisplayPluginMenu(HMENU hMenu, int nToolbarCmdID, POINT pt, UINT nMenuFlags)
 {
@@ -1402,13 +1293,11 @@
 
   Dictionary* dictionary = Dictionary::GetInstance();
 
-  MENUITEMINFOW fmii;
-  memset(&fmii, 0, sizeof(MENUITEMINFO));
-  fmii.cbSize = sizeof(MENUITEMINFO);
+  MENUITEMINFOW fmii = {};
+  fmii.cbSize = sizeof(fmii);
 
-  MENUITEMINFOW miiSep;
-  memset(&miiSep, 0, sizeof(MENUITEMINFO));
-  miiSep.cbSize = sizeof(MENUITEMINFO);
+  MENUITEMINFOW miiSep = {};
+  miiSep.cbSize = sizeof(miiSep);
   miiSep.fMask = MIIM_TYPE | MIIM_FTYPE;
   miiSep.fType = MFT_SEPARATOR;
 
@@ -1511,8 +1400,7 @@
 
     if (nButton > 0)
     {
-      TBBUTTON pTBBtn;
-      memset(&pTBBtn, 0, sizeof(TBBUTTON));
+      TBBUTTON pTBBtn = {};
 
       if (SendMessage(hWndToolBar, TB_GETBUTTON, nButton, (LPARAM)&pTBBtn))
       {
@@ -1941,7 +1829,7 @@
 
 HWND CPluginClass::GetTabHWND() const
 {
-  wchar_t szClassName[MAX_PATH];
+  std::array<wchar_t, MAX_PATH> className;
   // Get browser window and url
   HWND hBrowserWnd = GetBrowserHWND();
   if (!hBrowserWnd)
@@ -1957,14 +1845,14 @@
   HWND hTabWnd = ::GetWindow(hBrowserWnd, GW_CHILD);
   while (hTabWnd)
   {
-    memset(szClassName, 0, MAX_PATH);
-    GetClassName(hTabWnd, szClassName, MAX_PATH);
+    className.fill(L'\0');
+    GetClassName(hTabWnd, className.data(), className.size());
 
-    if (wcscmp(szClassName, L"TabWindowClass") == 0 || wcscmp(szClassName, L"Frame Tab") == 0)
+    if (wcscmp(className.data(), L"TabWindowClass") == 0 || wcscmp(className.data(), L"Frame Tab") == 0)
     {
       // IE8 support
       HWND hTabWnd2 = hTabWnd;
-      if (wcscmp(szClassName, L"Frame Tab") == 0)
+      if (wcscmp(className.data(), L"Frame Tab") == 0)
       {
         hTabWnd2 = ::FindWindowEx(hTabWnd2, NULL, L"TabWindowClass", NULL);
       }