From 4438e67298bd4466020c20446553c6c38dd2b7b3 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Wed, 2 Jan 2019 23:08:48 +0100 Subject: [PATCH 01/13] fix: squeakIniName cannot be both a WCHAR* (sqWin32Security.c) and TCHAR* Thus maintain the two variants squeakIniNameW and squeakIniNameA Also fix this invariant: when reading imageNameW from ini file, make sure the ASCII variant is initialized too --- .../plugins/SecurityPlugin/sqWin32Security.c | 11 ++++----- platforms/win32/vm/sqWin32.h | 4 ++++ platforms/win32/vm/sqWin32Main.c | 1 - platforms/win32/vm/sqWin32Prefs.c | 23 +++++++++---------- platforms/win32/vm/sqWin32Window.c | 23 ++++++++++--------- 5 files changed, 31 insertions(+), 31 deletions(-) diff --git a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c index bae6199cce..3a4d87cc4a 100644 --- a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c +++ b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c @@ -28,9 +28,6 @@ static char untrustedUserDirectoryUTF8[MAX_PATH]; static char secureUserDirectoryUTF8[MAX_PATH]; static char resourceDirectoryUTF8[MAX_PATH]; -/* imported from sqWin32Prefs.c */ -extern WCHAR squeakIniName[MAX_PATH]; - /* imported from sqWin32Main.c */ extern BOOL fLowRights; /* started as low integrity process, need to use alternate untrustedUserDirectory */ @@ -293,20 +290,20 @@ sqInt ioInitSecurity(void) { /* Query Squeak.ini for network installations */ GetPrivateProfileStringW(L"Security", L"SecureDirectory", secureUserDirectory, secureUserDirectory, - MAX_PATH, squeakIniName); + MAX_PATH, squeakIniNameW); if(fLowRights) {/* use alternate untrustedUserDirectory */ GetPrivateProfileStringW(L"Security", L"UserDirectoryLow", untrustedUserDirectory, untrustedUserDirectory, - MAX_PATH, squeakIniName); + MAX_PATH, squeakIniNameW); } else { GetPrivateProfileStringW(L"Security", L"UserDirectory", untrustedUserDirectory, untrustedUserDirectory, - MAX_PATH, squeakIniName); + MAX_PATH, squeakIniNameW); } GetPrivateProfileStringW(L"Security", L"ResourceDirectory", resourceDirectory, resourceDirectory, - MAX_PATH, squeakIniName); + MAX_PATH, squeakIniNameW); /* Attempt to read local user settings from registry */ ok = RegOpenKeyA(HKEY_CURRENT_USER, HKEY_SQUEAK_ROOT, &hk); diff --git a/platforms/win32/vm/sqWin32.h b/platforms/win32/vm/sqWin32.h index 4123ca98fd..3dcf32279e 100644 --- a/platforms/win32/vm/sqWin32.h +++ b/platforms/win32/vm/sqWin32.h @@ -269,17 +269,21 @@ extern WCHAR vmNameW[]; /* name of the interpreter's executable - UTF16 extern char windowTitle[]; /* window title string - UTF8 */ extern char vmBuildString[]; /* the vm build string */ extern TCHAR windowClassName[]; /* class name for the window */ +extern char squeakIniNameA[]; /* full path to ini file - UTF8 */ +extern WCHAR squeakIniNameW[]; /* full path to ini file - UTF16 */ #ifdef UNICODE #define imageNameT imageNameW /* define the generic TCHAR* version */ #define imagePath imagePathW #define vmName vmNameW #define vmPath vmPathW +#define squeakIniName squeakIniNameW #else #define imageNameT imageName #define imagePath imagePathA #define vmName vmNameA #define vmPath vmPathA +#define squeakIniName squeakIniNameA #endif extern UINT SQ_LAUNCH_DROP; diff --git a/platforms/win32/vm/sqWin32Main.c b/platforms/win32/vm/sqWin32Main.c index 51b91b2e9c..a70eef2469 100644 --- a/platforms/win32/vm/sqWin32Main.c +++ b/platforms/win32/vm/sqWin32Main.c @@ -94,7 +94,6 @@ int getFullScreenFlag(void); sqInt methodPrimitiveIndex(void); int getCurrentBytecode(void); -extern TCHAR squeakIniName[]; extern void printPhaseTime(int); /* Import from sqWin32Alloc.c */ diff --git a/platforms/win32/vm/sqWin32Prefs.c b/platforms/win32/vm/sqWin32Prefs.c index a2a5cc5645..fa94ac2e5c 100644 --- a/platforms/win32/vm/sqWin32Prefs.c +++ b/platforms/win32/vm/sqWin32Prefs.c @@ -37,7 +37,6 @@ void printCallStack(void); void printAllStacks(void); /* VM preference variables */ -extern TCHAR squeakIniName[]; /* full path and name to ini file */ HMENU vmPrefsMenu; /* preferences menu */ extern int caseSensitiveFileMode; @@ -188,25 +187,25 @@ void LoadPreferences() int size; /* make ini file name based on executable file name */ - lstrcpy(squeakIniName, vmName); - size = lstrlen(squeakIniName); - lstrcpy(squeakIniName + (size-3), TEXT("ini")); + wcscpy(squeakIniNameW, vmNameW); + size = wcslen(squeakIniNameW); + wcscpy(squeakIniNameW + (size-3), L"ini"); + WideCharToMultiByte(CP_UTF8, 0, squeakIniNameW, -1, squeakIniNameA, MAX_PATH_UTF8, NULL, NULL); /* get image file name from ini file */ - size = GetPrivateProfileString(U_GLOBAL, TEXT("ImageFile"), - TEXT(""), imageNameT, MAX_PATH, squeakIniName); + size = GetPrivateProfileStringW(L"Global", L"ImageFile", + L"", imageNameW, MAX_PATH, squeakIniNameW); if(size > 0) { - if( !(imageName[0] == '\\' && imageName[1] == '\\') && !(imageName[1] == ':' && imageName[2] == '\\')) { + if( !(imageNameW[0] == L'\\' && imageNameW[1] == L'\\') && !(imageNameW[1] == L':' && imageNameW[2] == L'\\')) { /* make the path relative to VM directory */ - strcpy(imageName , vmNameA); wcscpy(imageNameW, vmNameW); - (strrchr(imageName ,'\\' ))[1] = 0; (wcsrchr(imageNameW, W_BACKSLASH[0]))[1] = 0; - size = lstrlen(imageNameT); - size = GetPrivateProfileString(U_GLOBAL, TEXT("ImageFile"), - TEXT(""), imageNameT + size, MAX_PATH - size, squeakIniName); + size = wcslen(imageNameW); + size = GetPrivateProfileStringW(L"Global", L"ImageFile", + L"", imageNameW + size, MAX_PATH - size, squeakIniNameW); } } + WideCharToMultiByte(CP_UTF8, 0, imageNameW, -1, imageName, MAX_PATH_UTF8, NULL, NULL); /* get window title from ini file */ #ifdef UNICODE diff --git a/platforms/win32/vm/sqWin32Window.c b/platforms/win32/vm/sqWin32Window.c index 941d4d8048..b8a780d4c0 100644 --- a/platforms/win32/vm/sqWin32Window.c +++ b/platforms/win32/vm/sqWin32Window.c @@ -64,17 +64,18 @@ extern sqInt deferDisplayUpdates; - UTF16 version is for interaction with WIN32 API whichever code modifies one version is responsible for updating the other */ -char imageName [MAX_PATH_UTF8 + 1]; /* full path and name to image */ -WCHAR imageNameW[MAX_PATH + 1]; /* full path and name to image */ -char imagePathA[MAX_PATH_UTF8 + 1]; /* full path to image */ -WCHAR imagePathW[MAX_PATH + 1]; /* full path to image */ -char vmPathA[MAX_PATH_UTF8 + 1]; /* full path to interpreter's directory */ -WCHAR vmPathW[MAX_PATH + 1]; /* full path to interpreter's directory */ -char vmNameA[MAX_PATH_UTF8 + 1]; /* name of the interpreter's executable UTF8 */ -WCHAR vmNameW[MAX_PATH + 1]; /* name of the interpreter's executable UTF16 */ -char windowTitle[MAX_PATH+1]; /* what should we display in the title? */ -TCHAR squeakIniName[MAX_PATH+1]; /* full path and name to ini file */ -TCHAR windowClassName[MAX_PATH+1]; /* Window class name */ +char imageName [MAX_PATH_UTF8 + 1]; /* full path and name to image */ +WCHAR imageNameW[MAX_PATH + 1]; /* full path and name to image */ +char imagePathA[MAX_PATH_UTF8 + 1]; /* full path to image */ +WCHAR imagePathW[MAX_PATH + 1]; /* full path to image */ +char vmPathA[MAX_PATH_UTF8 + 1]; /* full path to interpreter's directory */ +WCHAR vmPathW[MAX_PATH + 1]; /* full path to interpreter's directory */ +char vmNameA[MAX_PATH_UTF8 + 1]; /* name of the interpreter's executable UTF8 */ +WCHAR vmNameW[MAX_PATH + 1]; /* name of the interpreter's executable UTF16 */ +char windowTitle[MAX_PATH+1]; /* what should we display in the title? */ +WCHAR squeakIniNameW[MAX_PATH + 1]; /* full path and name to ini file */ +char squeakIniNameA[MAX_PATH_UTF8 + 1]; /* full path and name to ini file */ +TCHAR windowClassName[MAX_PATH+1]; /* Window class name */ const TCHAR U_ON[] = TEXT("1"); const TCHAR U_OFF[] = TEXT("0"); From 76e2ecf05787caf3a90eb22bef622435a59c9617 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Wed, 2 Jan 2019 23:49:06 +0100 Subject: [PATCH 02/13] fix: remove superfluous TEXT macro we're in non UNICODE #ifdef branch, so the macro is void sprintf (ASCII) of some TEXT (TCHAR*) would not make sense anyway --- platforms/win32/vm/sqWin32Main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/platforms/win32/vm/sqWin32Main.c b/platforms/win32/vm/sqWin32Main.c index a70eef2469..913f329419 100644 --- a/platforms/win32/vm/sqWin32Main.c +++ b/platforms/win32/vm/sqWin32Main.c @@ -696,9 +696,9 @@ void gatherSystemInfo(void) { } #else snprintf(tmpString, sizeof(tmpString), - TEXT("Display Information: \n") - TEXT("\tGraphics adapter name: %s\n") - TEXT("\tPrimary monitor resolution: %d x %d\n"), + "Display Information: \n" + "\tGraphics adapter name: %s\n" + "\tPrimary monitor resolution: %d x %d\n", gDev.DeviceString, screenX, screenY); #endif From 3b06e9d50720adfb9c64dd5ddb06db032712dd69 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Wed, 2 Jan 2019 20:23:37 +0100 Subject: [PATCH 03/13] fix: wcsncpy does not necessarily writes the terminating NULL --- platforms/win32/vm/sqWin32Main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/platforms/win32/vm/sqWin32Main.c b/platforms/win32/vm/sqWin32Main.c index 913f329419..a57df62409 100644 --- a/platforms/win32/vm/sqWin32Main.c +++ b/platforms/win32/vm/sqWin32Main.c @@ -1674,6 +1674,7 @@ sqMain(int argc, char *argv[]) if(*imageNameW) { WCHAR path[MAX_PATH+1], *ptr; wcsncpy(path,imageNameW,MAX_PATH); + path[MAX_PATH] = 0; ptr = wcsrchr(path, '\\'); if(ptr) { *ptr = 0; From f7d1af84872a1ca1bed1b9c79ed0066f732ac406 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Wed, 2 Jan 2019 23:11:30 +0100 Subject: [PATCH 04/13] fix: B3DAccelerator: dwState is used un-initialized if value is not 0,1 or 2. --- .../win32/plugins/B3DAcceleratorPlugin/sqWin32D3D.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/platforms/win32/plugins/B3DAcceleratorPlugin/sqWin32D3D.c b/platforms/win32/plugins/B3DAcceleratorPlugin/sqWin32D3D.c index 74938d6156..6c5c532451 100644 --- a/platforms/win32/plugins/B3DAcceleratorPlugin/sqWin32D3D.c +++ b/platforms/win32/plugins/B3DAcceleratorPlugin/sqWin32D3D.c @@ -1077,9 +1077,12 @@ int d3dSetIntProperty(int handle, int prop, int value) ERROR_CHECK; return 1; case 2: /* polygon mode */ - if(value == 0) dwState = D3DFILL_SOLID; - if(value == 1) dwState = D3DFILL_WIREFRAME; - if(value == 2) dwState = D3DFILL_POINT; + switch (value) { + case 0: dwState = D3DFILL_SOLID; break; + case 1: dwState = D3DFILL_WIREFRAME; break; + case 2: dwState = D3DFILL_POINT; break; + default: return 0; + } hRes = lpDevice->lpVtbl-> SetRenderState(lpDevice, D3DRENDERSTATE_FILLMODE, dwState); ERROR_CHECK; From 0d88f639c063806b582220c77c533f65d174d707 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Wed, 2 Jan 2019 23:21:35 +0100 Subject: [PATCH 05/13] fix issue #336 potential buffer overrun in sqWin32Midi.c The cache dimension is only 128, so use mask & 0x7F to make sure that we do not overrun the cache. I don't know if it could happen, but now we are sure that it can't. --- platforms/win32/plugins/MIDIPlugin/sqWin32MIDI.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/platforms/win32/plugins/MIDIPlugin/sqWin32MIDI.c b/platforms/win32/plugins/MIDIPlugin/sqWin32MIDI.c index 3bceb4e7d2..201aba0aac 100644 --- a/platforms/win32/plugins/MIDIPlugin/sqWin32MIDI.c +++ b/platforms/win32/plugins/MIDIPlugin/sqWin32MIDI.c @@ -364,7 +364,7 @@ static void CALLBACK midiInCallback(HMIDIIN hMidiIn,UINT uMsg, DWORD_PTR dwUs { /* Cache controller values in the driver */ switch(cmd) { case ControlCmd: /* Read a control command */ - channel = (dwParam1 >> 8) & 0xFF; + channel = (dwParam1 >> 8) & 0x7F; value = (dwParam1 >> 16) & 0xFF; port->cache.sqControllers[channel] = value; return; @@ -375,7 +375,7 @@ static void CALLBACK midiInCallback(HMIDIIN hMidiIn,UINT uMsg, DWORD_PTR dwUs port->cache.sqPitchBend[channel] = (value2 << 7) + value; return; case PolyPressCmd: /* Read polyphonic key pressure */ - channel = (dwParam1 >> 8) & 0xFF; + channel = (dwParam1 >> 8) & 0x7F; value = (dwParam1 >> 16) & 0xFF; port->cache.sqKeyPressures[channel] = value; return; From eaa36cf28e397d939b96b95849f7c8d2eddc0688 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Wed, 2 Jan 2019 23:45:44 +0100 Subject: [PATCH 06/13] fix: use LoadLibraryA for -DUNICODE compatibility --- platforms/win32/vm/sqWin32Window.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/platforms/win32/vm/sqWin32Window.c b/platforms/win32/vm/sqWin32Window.c index b8a780d4c0..b95f8e0e28 100644 --- a/platforms/win32/vm/sqWin32Window.c +++ b/platforms/win32/vm/sqWin32Window.c @@ -2963,7 +2963,7 @@ int sqLaunchDrop(void) { Work around it for now. */ static LPWSTR* (WINAPI *sqCommandLineToArgvW)(LPCWSTR,int*) = NULL; if(!sqCommandLineToArgvW) { - HANDLE hShell32 = LoadLibrary("shell32.dll"); + HANDLE hShell32 = LoadLibraryA("shell32.dll"); sqCommandLineToArgvW=(void*)GetProcAddress(hShell32, "CommandLineToArgvW"); if(!sqCommandLineToArgvW) return 0; } From d066799e3ccff36ad96c2b8a65863aee7a4778d3 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Wed, 2 Jan 2019 23:46:53 +0100 Subject: [PATCH 07/13] fix: LoadLibrary could fail thus - hUser could be NULL - hPsApi may be NULL - hAdvApi32 might be NULL [skip travis] we only have platforms/win32 changes --- .../win32/plugins/CroquetPlugin/sqWin32CroquetPlugin.c | 1 + platforms/win32/vm/sqWin32Backtrace.c | 4 ++++ platforms/win32/vm/sqWin32Main.c | 8 +++++--- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/platforms/win32/plugins/CroquetPlugin/sqWin32CroquetPlugin.c b/platforms/win32/plugins/CroquetPlugin/sqWin32CroquetPlugin.c index 5c903130b4..f256473b72 100644 --- a/platforms/win32/plugins/CroquetPlugin/sqWin32CroquetPlugin.c +++ b/platforms/win32/plugins/CroquetPlugin/sqWin32CroquetPlugin.c @@ -9,6 +9,7 @@ int ioGatherEntropy(char *bufPtr, int bufSize) { if(!loaded) { loaded = 1; hAdvApi32 = LoadLibraryA("advapi32.dll"); + if (!hAdvApi32) return 0; RtlGenRandom = (void*)GetProcAddress(hAdvApi32, "SystemFunction036"); } if(!RtlGenRandom) return 0; diff --git a/platforms/win32/vm/sqWin32Backtrace.c b/platforms/win32/vm/sqWin32Backtrace.c index 86a69bc188..f9c0e1a2a9 100644 --- a/platforms/win32/vm/sqWin32Backtrace.c +++ b/platforms/win32/vm/sqWin32Backtrace.c @@ -264,6 +264,10 @@ get_modules(void) HANDLE hPsApi = LoadLibraryA("psapi.dll"); HMODULE *modules; + if (!hPsApi) { + printLastError(TEXT("LoadLibrary psapi")); + return; + } EnumProcessModules = (void*)GetProcAddress(hPsApi, "EnumProcessModules"); GetModuleInformation=(void*)GetProcAddress(hPsApi, "GetModuleInformation"); diff --git a/platforms/win32/vm/sqWin32Main.c b/platforms/win32/vm/sqWin32Main.c index a57df62409..4adcf3ded9 100644 --- a/platforms/win32/vm/sqWin32Main.c +++ b/platforms/win32/vm/sqWin32Main.c @@ -541,11 +541,13 @@ void gatherSystemInfo(void) { { HANDLE hUser = LoadLibraryA( "user32.dll" ); - pfnEnumDisplayDevices pEnumDisplayDevices = (pfnEnumDisplayDevices) - GetProcAddress(hUser, "EnumDisplayDevicesA"); ZeroMemory(&gDev, sizeof(gDev)); gDev.cb = sizeof(gDev); - if (pEnumDisplayDevices) pEnumDisplayDevices(NULL, 0, &gDev, 0); + if(hUser) { + pfnEnumDisplayDevices pEnumDisplayDevices = (pfnEnumDisplayDevices) + GetProcAddress(hUser, "EnumDisplayDevicesA"); + if (pEnumDisplayDevices) pEnumDisplayDevices(NULL, 0, &gDev, 0); + } } { /* Figure out make and model from OEMINFO.ini */ From 2bfdddc8ee084394d69ff3b95c93d2850a2716ca Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Thu, 3 Jan 2019 00:39:40 +0100 Subject: [PATCH 08/13] fix: SecurityPlugin: RegQueryValueExW dwSize is buffer size in bytes Therefore, it is not the number of characters (WCHAR), but rather twice that number! --- .../plugins/SecurityPlugin/sqWin32Security.c | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c index 3a4d87cc4a..a99ed67a6e 100644 --- a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c +++ b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c @@ -309,37 +309,37 @@ sqInt ioInitSecurity(void) { ok = RegOpenKeyA(HKEY_CURRENT_USER, HKEY_SQUEAK_ROOT, &hk); /* Read the secure directory from the subkey. */ - dwSize = MAX_PATH; + dwSize = MAX_PATH*sizeof(WCHAR); ok = RegQueryValueExW(hk, L"SecureDirectory",NULL, &dwType, (LPBYTE) tmp, &dwSize); if(ok == ERROR_SUCCESS) { - if(tmp[dwSize-2] != '\\') { - tmp[dwSize-1] = '\\'; - tmp[dwSize] = 0; + if(tmp[dwSize/2-2] != '\\') { + tmp[dwSize/2-1] = '\\'; + tmp[dwSize/2] = 0; } lstrcpyW(secureUserDirectory, tmp); } /* Read the user directory from the subkey. */ - dwSize = MAX_PATH; + dwSize = MAX_PATH*sizeof(WCHAR); ok = RegQueryValueExW(hk, L"UserDirectory",NULL, &dwType, (LPBYTE) tmp, &dwSize); if(ok == ERROR_SUCCESS) { - if(tmp[dwSize-2] != '\\') { - tmp[dwSize-1] = '\\'; - tmp[dwSize] = 0; + if(tmp[dwSize/2-2] != '\\') { + tmp[dwSize/2-1] = '\\'; + tmp[dwSize/2] = 0; } lstrcpyW(untrustedUserDirectory, tmp); } /* Read the resource directory from the subkey. */ - dwSize = MAX_PATH; + dwSize = MAX_PATH*sizeof(WCHAR); ok = RegQueryValueExW(hk, L"ResourceDirectory",NULL, &dwType, (LPBYTE) tmp, &dwSize); if(ok == ERROR_SUCCESS) { - if(tmp[dwSize-2] != '\\') { - tmp[dwSize-1] = '\\'; - tmp[dwSize] = 0; + if(tmp[dwSize/2-2] != '\\') { + tmp[dwSize/2-1] = '\\'; + tmp[dwSize/2] = 0; } lstrcpyW(resourceDirectory, tmp); } From bf754ec9b8c8ffec1ac958ed12e897c22ae1ce18 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Thu, 3 Jan 2019 01:01:42 +0100 Subject: [PATCH 09/13] SecurityPlugin: prefer wcs* functions to lstr*W It's more in line with the style of platforms/win32 (though it's still quite noisy) --- .../plugins/SecurityPlugin/sqWin32Security.c | 69 ++++++++----------- 1 file changed, 27 insertions(+), 42 deletions(-) diff --git a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c index a99ed67a6e..c1e521d720 100644 --- a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c +++ b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c @@ -68,27 +68,12 @@ static int testDotDot(WCHAR *pathName, int index) { return 1; } -static int lstrncmpW(WCHAR *s1, WCHAR *s2, int len) { - int s1Len = lstrlenW(s1); - int s2Len = lstrlenW(s2); - int max = min(s1Len, min(s2Len, len)); - int i; - for (i = 0; i < max; i++) { - if (s1[i] > s2[i]) { - return 1; - } else if (s1[i] < s2[i]) { - return -1; - } - } - return 0; -} - static int isAccessiblePathName(WCHAR *pathName, int writeFlag) { - int pathLen = lstrlenW(pathName); + int pathLen = wcslen(pathName); if (pathLen > (MAX_PATH - 1)) return 0; if (pathLen >= untrustedUserDirectoryLen - && 0 == lstrncmpW(pathName, untrustedUserDirectory, untrustedUserDirectoryLen)) { + && 0 == wcsncmp(pathName, untrustedUserDirectory, untrustedUserDirectoryLen)) { if (pathLen > untrustedUserDirectoryLen + 2) return testDotDot(pathName, untrustedUserDirectoryLen+2); return 1; @@ -97,7 +82,7 @@ static int isAccessiblePathName(WCHAR *pathName, int writeFlag) { return 0; if (pathLen >= resourceDirectoryLen - && 0 == lstrncmpW(pathName, resourceDirectory, resourceDirectoryLen)) { + && 0 == wcsncmp(pathName, resourceDirectory, resourceDirectoryLen)) { if (pathLen > resourceDirectoryLen + 2) return testDotDot(pathName, resourceDirectoryLen+2); return 1; @@ -221,13 +206,13 @@ int expandMyDocuments(WCHAR *pathname, WCHAR *replacement, WCHAR *result) /* WCHAR search4[MAX_PATH+1]; WCHAR *start; - lstrcpyW(search4, L"%MYDOCUMENTS%"); + wcscpy(search4, L"%MYDOCUMENTS%"); - if(!(start = wstrstr(pathname, search4))) return 0; + if(!(start = wcsstr(pathname, search4))) return 0; - wstrncpy(result, pathname, start-pathname); - result[start-pathname] = '\0'; - sprintf(result+(start-pathname),"%s%s", replacement, start+lstrlenW(search4)); + wcsncpy(result, pathname, start-pathname); + result[start-pathname] = L'\0'; + swprintf(result+(start-pathname),L"%s%s", replacement, start+wcslen(search4)); */ /* TODO: Implement this properly. */ return 0; @@ -236,12 +221,12 @@ int expandMyDocuments(WCHAR *pathname, WCHAR *replacement, WCHAR *result) static void expandVariableInDirectory(WCHAR *directory, WCHAR *wDir, WCHAR *wTmp) { /* Expand environment variables. */ - lstrcpyW(wDir, directory); + wcscpy(wDir, directory); ExpandEnvironmentStringsW(wDir, wTmp, MAX_PATH - 1); /* Expand relative paths to absolute paths */ GetFullPathNameW(wTmp, MAX_PATH, wDir, NULL); - lstrcpyW(directory, wDir); + wcscpy(directory, wDir); } /* note: following is called from VM directly, not from plugin */ @@ -256,17 +241,17 @@ sqInt ioInitSecurity(void) { /* establish the secure user directory */ sqUTF8ToUTF16Copy(secureUserDirectory, sizeof(secureUserDirectory)/sizeof(secureUserDirectory[0]), sqGetCurrentImagePath()); - dirLen = lstrlenW(secureUserDirectory); + dirLen = wcslen(secureUserDirectory); dwSize = MAX_PATH-dirLen; GetUserNameW(secureUserDirectory+dirLen, &dwSize); /* establish untrusted user directory */ - lstrcpyW(untrustedUserDirectory, L"C:\\My Squeak\\%USERNAME%"); + wcscpy(untrustedUserDirectory, L"C:\\My Squeak\\%USERNAME%"); /* establish untrusted user directory */ sqUTF8ToUTF16Copy(resourceDirectory, sizeof(resourceDirectory) / sizeof(resourceDirectory[0]), sqGetCurrentImagePath()); - if (resourceDirectory[lstrlenW(resourceDirectory)-1] == '\\') { - resourceDirectory[lstrlenW(resourceDirectory)-1] = 0; + if (resourceDirectory[wcslen(resourceDirectory)-1] == '\\') { + resourceDirectory[wcslen(resourceDirectory)-1] = 0; } /* Look up shGetFolderPathW */ @@ -278,11 +263,11 @@ sqInt ioInitSecurity(void) { int sz; /*shGetfolderPath does not return utf8*/ if(shGetFolderPath(NULL, CSIDL_PERSONAL, NULL, 0, untrustedUserDirectory) == S_OK) { - sz = lstrlenW(untrustedUserDirectory); + sz = wcslen(untrustedUserDirectory); if(untrustedUserDirectory[sz-1] != '\\') - lstrcatW(untrustedUserDirectory, L"\\"); - lstrcpyW(myDocumentsFolder,untrustedUserDirectory); - lstrcatW(untrustedUserDirectory, L"My Squeak"); + wcscat(untrustedUserDirectory, L"\\"); + wcscpy(myDocumentsFolder,untrustedUserDirectory); + wcscat(untrustedUserDirectory, L"My Squeak"); } } @@ -317,7 +302,7 @@ sqInt ioInitSecurity(void) { tmp[dwSize/2-1] = '\\'; tmp[dwSize/2] = 0; } - lstrcpyW(secureUserDirectory, tmp); + wcscpy(secureUserDirectory, tmp); } /* Read the user directory from the subkey. */ @@ -329,7 +314,7 @@ sqInt ioInitSecurity(void) { tmp[dwSize/2-1] = '\\'; tmp[dwSize/2] = 0; } - lstrcpyW(untrustedUserDirectory, tmp); + wcscpy(untrustedUserDirectory, tmp); } /* Read the resource directory from the subkey. */ @@ -341,7 +326,7 @@ sqInt ioInitSecurity(void) { tmp[dwSize/2-1] = '\\'; tmp[dwSize/2] = 0; } - lstrcpyW(resourceDirectory, tmp); + wcscpy(resourceDirectory, tmp); } RegCloseKey(hk); @@ -349,15 +334,15 @@ sqInt ioInitSecurity(void) { if(shGetFolderPath) { dwSize = expandMyDocuments(untrustedUserDirectory, myDocumentsFolder, tmp); if(dwSize > 0 && dwSize < MAX_PATH) - lstrcpyW(untrustedUserDirectory, tmp); + wcscpy(untrustedUserDirectory, tmp); dwSize = expandMyDocuments(secureUserDirectory, myDocumentsFolder, tmp); if(dwSize > 0 && dwSize < MAX_PATH) - lstrcpyW(secureUserDirectory, tmp); + wcscpy(secureUserDirectory, tmp); dwSize = expandMyDocuments(resourceDirectory, myDocumentsFolder, tmp); if(dwSize > 0 && dwSize < MAX_PATH) - lstrcpyW(resourceDirectory, tmp); + wcscpy(resourceDirectory, tmp); } /* Expand the directories. */ @@ -365,9 +350,9 @@ sqInt ioInitSecurity(void) { expandVariableInDirectory(secureUserDirectory, wDir, wTmp); expandVariableInDirectory(resourceDirectory, wDir, wTmp); - secureUserDirectoryLen = lstrlenW(secureUserDirectory); - untrustedUserDirectoryLen = lstrlenW(untrustedUserDirectory); - resourceDirectoryLen = lstrlenW(resourceDirectory); + secureUserDirectoryLen = wcslen(secureUserDirectory); + untrustedUserDirectoryLen = wcslen(untrustedUserDirectory); + resourceDirectoryLen = wcslen(resourceDirectory); /* Keep a UTF-8 copy*/ sqUTF16ToUTF8Copy(untrustedUserDirectoryUTF8, sizeof(untrustedUserDirectoryUTF8), untrustedUserDirectory); From 01cbedd1a13ccf57216d9c6662bb40e5197a46b7 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Thu, 3 Jan 2019 00:45:02 +0100 Subject: [PATCH 10/13] Fix: securityPlugin: use WideCharToMultiByte and CP_UTF8 It's far more correct than naive sqUTF16toUTF8Copy. This function should be removed. Therefore, use MAX_PATH_UTF8 to be sure that there is enough room in UTF8 encoded strings. Add a guard of 1 character to handle terminating NULL --- .../plugins/SecurityPlugin/sqWin32Security.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c index c1e521d720..34ee2c9a65 100644 --- a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c +++ b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c @@ -17,16 +17,16 @@ static HRESULT (__stdcall *shGetFolderPath)(HWND, int, HANDLE, DWORD, WCHAR*); -static WCHAR untrustedUserDirectory[MAX_PATH]; +static WCHAR untrustedUserDirectory[MAX_PATH+1]; static int untrustedUserDirectoryLen; -static WCHAR secureUserDirectory[MAX_PATH]; +static WCHAR secureUserDirectory[MAX_PATH+1]; static int secureUserDirectoryLen; -static WCHAR resourceDirectory[MAX_PATH]; +static WCHAR resourceDirectory[MAX_PATH+1]; static int resourceDirectoryLen; -static char untrustedUserDirectoryUTF8[MAX_PATH]; -static char secureUserDirectoryUTF8[MAX_PATH]; -static char resourceDirectoryUTF8[MAX_PATH]; +static char untrustedUserDirectoryUTF8[MAX_PATH_UTF8+1]; +static char secureUserDirectoryUTF8[MAX_PATH_UTF8+1]; +static char resourceDirectoryUTF8[MAX_PATH_UTF8+1]; /* imported from sqWin32Main.c */ extern BOOL fLowRights; /* started as low integrity process, @@ -355,9 +355,9 @@ sqInt ioInitSecurity(void) { resourceDirectoryLen = wcslen(resourceDirectory); /* Keep a UTF-8 copy*/ - sqUTF16ToUTF8Copy(untrustedUserDirectoryUTF8, sizeof(untrustedUserDirectoryUTF8), untrustedUserDirectory); - sqUTF16ToUTF8Copy(secureUserDirectoryUTF8, sizeof(secureUserDirectoryUTF8), secureUserDirectory); - sqUTF16ToUTF8Copy(resourceDirectoryUTF8, sizeof(resourceDirectoryUTF8), resourceDirectory); + WideCharToMultiByte(CP_UTF8, 0, untrustedUserDirectory, -1, untrustedUserDirectoryUTF8, MAX_PATH_UTF8, NULL, NULL); + WideCharToMultiByte(CP_UTF8, 0, secureUserDirectory , -1, secureUserDirectoryUTF8 , MAX_PATH_UTF8, NULL, NULL); + WideCharToMultiByte(CP_UTF8, 0, resourceDirectory , -1, resourceDirectoryUTF8 , MAX_PATH_UTF8, NULL, NULL); return 1; } From ec2e7e96377379bae13ba8e71e2cb7395ecd5a02 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Thu, 3 Jan 2019 01:27:16 +0100 Subject: [PATCH 11/13] SecurityPlugin use MultiByteToWideChar rather than sqUTF8toUTF16Copy The former is the standard and correct implementation The later is a naive stub Note: as an internal plugin, we could rather just copy imagePathW... [skip travis] because sole changes are in platforms/win32 --- platforms/win32/plugins/SecurityPlugin/sqWin32Security.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c index 34ee2c9a65..a7a8be77b4 100644 --- a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c +++ b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c @@ -240,7 +240,7 @@ sqInt ioInitSecurity(void) { int dirLen; /* establish the secure user directory */ - sqUTF8ToUTF16Copy(secureUserDirectory, sizeof(secureUserDirectory)/sizeof(secureUserDirectory[0]), sqGetCurrentImagePath()); + MultiByteToWideChar(CP_UTF8,0, sqGetCurrentImagePath(),-1,secureUserDirectory, MAX_PATH ); dirLen = wcslen(secureUserDirectory); dwSize = MAX_PATH-dirLen; GetUserNameW(secureUserDirectory+dirLen, &dwSize); @@ -249,7 +249,7 @@ sqInt ioInitSecurity(void) { wcscpy(untrustedUserDirectory, L"C:\\My Squeak\\%USERNAME%"); /* establish untrusted user directory */ - sqUTF8ToUTF16Copy(resourceDirectory, sizeof(resourceDirectory) / sizeof(resourceDirectory[0]), sqGetCurrentImagePath()); + MultiByteToWideChar(CP_UTF8, 0, sqGetCurrentImagePath(), -1, resourceDirectory, MAX_PATH); if (resourceDirectory[wcslen(resourceDirectory)-1] == '\\') { resourceDirectory[wcslen(resourceDirectory)-1] = 0; } From 9b485667f433a2863a87452712c58b692fe79e8c Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Thu, 3 Jan 2019 09:42:32 +0100 Subject: [PATCH 12/13] fix: SecurityPlugin: restore expandMyDocuments Note: this function was temporarily disabled in minheadless merge. Note: There was no provision for MAX_PATH buffer overrun (in fact all the arrays are MAX_PATH+1 long). Now abort the replacement when such overrun happens. This is not satisfying, we should signal the error... [skip travis] --- .../win32/plugins/SecurityPlugin/sqWin32Security.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c index a7a8be77b4..c7db7b9cde 100644 --- a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c +++ b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c @@ -203,8 +203,9 @@ char *ioGetUntrustedUserDirectory(void) { int expandMyDocuments(WCHAR *pathname, WCHAR *replacement, WCHAR *result) { -/* WCHAR search4[MAX_PATH+1]; + WCHAR search4[MAX_PATH+1]; WCHAR *start; + int len; wcscpy(search4, L"%MYDOCUMENTS%"); @@ -212,10 +213,13 @@ int expandMyDocuments(WCHAR *pathname, WCHAR *replacement, WCHAR *result) wcsncpy(result, pathname, start-pathname); result[start-pathname] = L'\0'; - swprintf(result+(start-pathname),L"%s%s", replacement, start+wcslen(search4)); -*/ - /* TODO: Implement this properly. */ - return 0; + len = _snwprintf(result+(start-pathname),MAX_PATH-(start - pathname),L"%s%s", replacement, start+wcslen(search4)); + + if (len < 0) { /* handle failure when replacement is too long: abort the replacement... what should we do? */ + result[MAX_PATH]=0; + return 0; + } else + return len + (start - pathname); } static void expandVariableInDirectory(WCHAR *directory, WCHAR *wDir, WCHAR *wTmp) From adbdf2e6511c89a7206bde7265024b23d5ca75b8 Mon Sep 17 00:00:00 2001 From: Nicolas Cellier Date: Thu, 3 Jan 2019 12:42:13 +0100 Subject: [PATCH 13/13] fix SecurityPlugin: revert unfortunate copy/paste This was from minheadless merge [skip travis] --- platforms/win32/plugins/SecurityPlugin/sqWin32Security.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c index c7db7b9cde..098da43b49 100644 --- a/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c +++ b/platforms/win32/plugins/SecurityPlugin/sqWin32Security.c @@ -388,7 +388,7 @@ int _ioSetImageWrite(int enable) { int _ioSetFileAccess(int enable) { if(enable == allowFileAccess) return 1; if(!allowFileAccess) { - if (!sqAskSecurityYesNoQuestion("WARNING: Re-enabling the ability to write the image is a serious security hazard. Do you want to continue?")) + if (!sqAskSecurityYesNoQuestion("WARNING: Re-enabling the ability to access arbitrary files is a serious security hazard. Do you want to continue?")) return 0; if (!sqAskSecurityYesNoQuestion("WARNING: Untrusted code could WIPE OUT your entire hard disk, STEAL your credit card information and send your PERSONAL documents to the entire world. Do you really want to continue?")) return 0; @@ -402,7 +402,7 @@ int _ioSetFileAccess(int enable) { int _ioSetSocketAccess(int enable) { if(enable == allowSocketAccess) return 1; if(!allowSocketAccess) { - if (!sqAskSecurityYesNoQuestion("WARNING: Re-enabling the ability to write the image is a serious security hazard. Do you want to continue?")) + if (!sqAskSecurityYesNoQuestion("WARNING: Re-enabling the ability to use sockets is a serious security hazard. Do you want to continue?")) return 0; if (!sqAskSecurityYesNoQuestion("WARNING: Untrusted code could WIPE OUT your entire hard disk, STEAL your credit card information and send your PERSONAL documents to the entire world. Do you really want to continue?")) return 0;