Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix #960

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
105 changes: 0 additions & 105 deletions .gitignore

This file was deleted.

6 changes: 4 additions & 2 deletions src/Common/BaseCom.cpp
Expand Up @@ -27,7 +27,10 @@ HRESULT CreateElevatedComObject (HWND hwnd, REFGUID guid, REFIID iid, void **ppv
WCHAR clsid[1024];
BIND_OPTS3 bo;

StringFromGUID2 (guid, clsid, sizeof (clsid) / 2);
if (!StringFromGUID2(guid, clsid, sizeof(clsid) / 2))
{
return E_OUTOFMEMORY;
}
swprintf_s (monikerName, sizeof (monikerName) / 2, L"Elevation:Administrator!new:%s", clsid);

memset (&bo, 0, sizeof (bo));
Expand Down Expand Up @@ -168,7 +171,6 @@ DWORD BaseCom::GetFileSize (BSTR filePath, unsigned __int64 *pSize)

try
{
std::wstring path (filePath);
File file(filePath, true);
file.CheckOpened (SRC_POS);
file.GetFileSize (*pSize);
Expand Down
20 changes: 10 additions & 10 deletions src/Common/BootEncryption.cpp
Expand Up @@ -1283,7 +1283,7 @@ namespace VeraCrypt

// Convert absolute path to relative to the Windows directory
driverPath = pathBuf;
driverPath = driverPath.substr (driverPath.rfind (L"\\", driverPath.rfind (L"\\", driverPath.rfind (L"\\") - 1) - 1) + 1);
driverPath = driverPath.substr (driverPath.rfind (L'\\', driverPath.rfind (L'\\', driverPath.rfind(L'\\') - 1) - 1) + 1);
}
}

Expand Down Expand Up @@ -4511,9 +4511,10 @@ namespace VeraCrypt
device.Read (bootLoaderBuf, sizeof (bootLoaderBuf));

// Prevent TrueCrypt loader from being backed up
for (size_t i = 0; i < sizeof (bootLoaderBuf) - strlen (TC_APP_NAME); ++i)
size_t lenAppName = strlen(TC_APP_NAME);
for (size_t i = 0; i < sizeof (bootLoaderBuf) - lenAppName; ++i)
{
if (memcmp (bootLoaderBuf + i, TC_APP_NAME, strlen (TC_APP_NAME)) == 0)
if (memcmp (bootLoaderBuf + i, TC_APP_NAME, lenAppName) == 0)
{
if (AskWarnNoYes ("TC_BOOT_LOADER_ALREADY_INSTALLED", ParentWindow) == IDNO)
throw UserAbort (SRC_POS);
Expand Down Expand Up @@ -4696,17 +4697,19 @@ namespace VeraCrypt
if (mszDest && input)
{
DWORD offset, remainingSize = dwDestSize;
DWORD str_input = strlen(input);
DWORD str_mszDest = strlen(mszDest);
Comment on lines +4700 to +4701
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two still have snake case (or even a mixed one for str_mszDest), so my previous comment about variable name casing is still actual. Moreover, the naming of those variables is still misleading (str_input suggests it's a "string of something we got as an input" or some such, and this is not the case). Using len prefix for the app name string length was good, so I'd suggest to do the same here.

In addition I did not repeat the comment for other variables and files for brevity in my previous review, but it's also applicable to other changes in this PR (e.g. str_len introduced in Dlgcode.c) - please correct them all.

while (*mszDest)
{
if (_stricmp (mszDest, input) == 0)
{
offset = (DWORD) strlen (input) + 1;
offset = str_input + 1;
memmove (mszDest, mszDest + offset, remainingSize - offset);
dwDestSize -= offset;
bRet = true;
break;
}
offset = (DWORD) strlen (mszDest) + 1;
offset = str_mszDest + 1;
mszDest += offset;
remainingSize -= offset;
}
Expand Down Expand Up @@ -5421,11 +5424,8 @@ namespace VeraCrypt
}

// Change the PKCS-5 PRF if requested by user
if (pkcs5 != 0)
{
cryptoInfo->pkcs5 = pkcs5;
RandSetHashFunction (pkcs5);
}
cryptoInfo->pkcs5 = pkcs5;
RandSetHashFunction (pkcs5);
wendig0x marked this conversation as resolved.
Show resolved Hide resolved

if (Randinit() != 0)
{
Expand Down
19 changes: 12 additions & 7 deletions src/Common/Dlgcode.c
Expand Up @@ -8031,13 +8031,14 @@ BOOL CALLBACK MultiChoiceDialogProc (HWND hwndDlg, UINT uMsg, WPARAM wParam, LPA
// Determine the number of newlines contained in the message text
{
int64 offset = -1;
int str_len = wcslen(L"\n");

do
{
offset = FindString ((char *) (bResolve ? GetString(*(pStrOrig+1)) : *(pwStrOrig+1)),
(char *) L"\n",
nMainTextLenInChars * 2,
(int) wcslen (L"\n") * 2,
str_len * 2,
offset + 1);

newLineSeqCount++;
Expand Down Expand Up @@ -14552,10 +14553,14 @@ static bool RunAsDesktopUser(
goto cleanup;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My previous comment about thise piece of code is still not resolved. dwLastError needs to be set in case of such an early exit, for the cleanup logic around SetLastError() to work properly.

}

AdjustTokenPrivileges(hThreadToken, FALSE, &tkp, 0, NULL, NULL);
dwLastErr = GetLastError();
if (ERROR_SUCCESS != dwLastErr)
if (!AdjustTokenPrivileges(hThreadToken, FALSE, &tkp, 0, NULL, NULL))
{
dwLastErr = GetLastError();
goto cleanup;
}
if (GetLastError() == ERROR_NOT_ALL_ASSIGNED)
{
dwLastErr = ERROR_NOT_ALL_ASSIGNED;
Comment on lines +14556 to +14563
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are not really necessary, the previous logic covered all the error paths and did it more concisely at that.

goto cleanup;
}
}
Expand Down Expand Up @@ -14638,7 +14643,7 @@ static bool RunAsDesktopUser(
if (hThreadToken) CloseHandle(hThreadToken);

if (!RevertToSelf())
return false;
return false;

if (!retval)
SetLastError (dwLastErr);
Expand Down Expand Up @@ -15263,7 +15268,7 @@ void PasswordEditDropTarget::GotLeave(void)
DWORD PasswordEditDropTarget::GotEnter(void)
{
TCHAR szClassName[64];
DWORD dwStyles;
LONG_PTR dwStyles;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. There's another one in ::GotDrop further below, still not fixed.

int maxLen;
HWND hChild = WindowFromPoint (m_DropPoint);
// check that we are on password edit control (we use maximum length to correctly identify password fields since they don't always have ES_PASSWORD style (if the the user checked show password)
Expand Down Expand Up @@ -15668,4 +15673,4 @@ bool OneOfKBsInstalled (const wchar_t* szKBs[], int count)

return bRet;
}
#endif // VC_COMREG
#endif // VC_COMREG
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this irrelevant whitespace change to avoid polluting the change history.

4 changes: 3 additions & 1 deletion src/Common/Xml.c
Expand Up @@ -93,9 +93,11 @@ char *XmlGetAttributeText (char *xmlNode, const char *xmlAttrName, char *xmlAttr
e = strchr (e, '>');
if (e == NULL) return NULL;

size_t len_xmlAttrName = strlen(xmlAttrName);

while ((t = strstr (t, xmlAttrName)) && t < e)
{
char *o = t + strlen (xmlAttrName);
char *o = t + len_xmlAttrName;
if (t[-1] == ' '
&&
(BeginsWith (o, "=\"")
Expand Down
4 changes: 3 additions & 1 deletion src/Format/Tcformat.c
Expand Up @@ -1515,7 +1515,9 @@ static void VerifySizeAndUpdate (HWND hwndDlg, BOOL bUpdate)

GetWindowText (GetDlgItem (hwndDlg, IDC_SIZEBOX), szTmp, ARRAYSIZE (szTmp));

for (i = 0; i < (__int64) wcslen (szTmp); i++)
__int64 len_szTmp = wcslen(szTmp);

for (i = 0; i < len_szTmp; i++)
{
if (szTmp[i] >= L'0' && szTmp[i] <= L'9')
continue;
Expand Down