From 43e28513e1dd446d8536c54a5b05cde56d38d191 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 17 Apr 2015 09:09:48 -0400 Subject: [PATCH] Make GetTempPathA/W first check TMPDIR Today GetTempPathA/W are hardcoded to return /tmp/. This commit makes them first consult the TMPDIR environment variable, using its value if it exists. This also overhauls the tests for GetTempPathA/W, which were out-of-date with regards to the existing implementation and which were excluded from PAL test runs. --- src/pal/src/file/path.cpp | 110 +++++++++----- .../file_io/GetTempPathW/test1/GetTempPathW.c | 157 ++++++++------------ .../file_io/gettemppatha/test1/gettemppatha.c | 160 ++++++++------------- src/pal/tests/palsuite/paltestlist.txt | 2 + 4 files changed, 195 insertions(+), 234 deletions(-) diff --git a/src/pal/src/file/path.cpp b/src/pal/src/file/path.cpp index 81ee86a..e85104f 100644 --- a/src/pal/src/file/path.cpp +++ b/src/pal/src/file/path.cpp @@ -440,18 +440,16 @@ Function: See MSDN. Notes: - On Windows NT/2000, the temp path is determined by the following steps: + On Windows, the temp path is determined by the following steps: 1. The value of the "TMP" environment variable, or if it doesn't exist, 2. The value of the "TEMP" environment variable, or if it doesn't exist, 3. The Windows directory. - - On the Mac, in the default environment, none of the above variables are - set, so the Temporary Items folder is used instead. - - Also note that dwPathLen will always be the proper return value, since - GetEnvironmentVariable and MultiByteToWideChar will both return the - required size of the buffer if they fail, which is what this function - should do also. + + On Unix, we follow in spirit: + 1. The value of the "TMPDIR" environment variable, or if it doesn't exist, + 2. The /tmp directory. + This is the same approach employed by mktemp. + --*/ DWORD PALAPI @@ -474,24 +472,66 @@ GetTempPathA( return 0; } - // GetTempPath is supposed to include the trailing slash. - const char *defaultDir = "/tmp/"; - - /* still no luck, use /tmp */ - if ( strlen(defaultDir) < nBufferLength ) + /* Try the TMPDIR environment variable. This is the same env var checked by mktemp. */ + dwPathLen = GetEnvironmentVariableA("TMPDIR", lpBuffer, nBufferLength); + if (dwPathLen > 0) { - dwPathLen = strlen(defaultDir); - strcpy_s(lpBuffer, nBufferLength, defaultDir); + /* The env var existed. dwPathLen will be the length without null termination + * if the entire value was successfully retrieved, or it'll be the length + * required to store the value with null termination. + */ + if (dwPathLen + 1 < nBufferLength) + { + /* We have enough space for the value whether it's '/'-terminated or not, so make sure it is. */ + if (lpBuffer[dwPathLen - 1] != '/') + { + lpBuffer[dwPathLen++] = '/'; + lpBuffer[dwPathLen] = '\0'; + } + } + else if (dwPathLen < nBufferLength) + { + /* We have enough space for the value, but only if it's already '/'-terminated. + * If it's not, we need to report enough space to hold the value, plus the '/' + * and a null terminator, which is not included in the originally reported length. + */ + if (lpBuffer[dwPathLen - 1] != '/') + { + dwPathLen += 2; + } + } + else /* dwPathLen >= nBufferLength */ + { + /* The value is too long for the supplied buffer. dwPathLen will now be the + * length required to hold the value, but we don't know whether that value + * is going to be '/' terminated. Since we'll need enough space for the '/', and since + * a caller would assume that the dwPathLen we return will be sufficient, + * we make sure to account for it in dwPathLen even if that means we end up saying + * one more byte of space is needed than actually is. + */ + dwPathLen++; + } } - else + else /* env var not found or was empty */ { - /* get the required length */ - dwPathLen = strlen(defaultDir) + 1; + /* no luck, use /tmp/ */ + const char *defaultDir = "/tmp/"; + int defaultDirLen = strlen(defaultDir); + if (defaultDirLen < nBufferLength) + { + dwPathLen = defaultDirLen; + strcpy_s(lpBuffer, nBufferLength, defaultDir); + } + else + { + /* get the required length */ + dwPathLen = defaultDirLen + 1; + } } - if ( dwPathLen > nBufferLength ) + if ( dwPathLen >= nBufferLength ) { - ERROR("Buffer is too small, need %d characters\n", dwPathLen); + ERROR("Buffer is too small, need space for %d characters including null termination\n", dwPathLen); SetLastError( ERROR_INSUFFICIENT_BUFFER ); } @@ -513,26 +553,24 @@ GetTempPathW( IN DWORD nBufferLength, OUT LPWSTR lpBuffer) { - char TempBuffer[ MAX_PATH ]; - DWORD dwRetVal = 0; - PERF_ENTRY(GetTempPathW); - ENTRY( "GetTempPathW(nBufferLength=%u, lpBuffer=%p)\n", - nBufferLength, lpBuffer); + ENTRY("GetTempPathW(nBufferLength=%u, lpBuffer=%p)\n", + nBufferLength, lpBuffer); - dwRetVal = GetTempPathA( MAX_PATH, TempBuffer ); - - // MAX_PATH should be big enough to hold whatever path, but due to concerns about multiple platforms, implememtation change inside GetTempPathA, and security attacks, adding checks for dwRetVal > MAX_PATH is better - if ( dwRetVal > MAX_PATH ) + if (!lpBuffer) { - // If dwRetVal > MAX_PATH, dwRetVal = required number of TCHARs for TempBuffer, including NULL - ERROR( "internal TempBuffer was not large enough.\n " ) - SetLastError( ERROR_INSUFFICIENT_BUFFER ); - *lpBuffer = '\0'; + ERROR("lpBuffer was not a valid pointer.\n") + SetLastError(ERROR_INVALID_PARAMETER); + LOGEXIT("GetTempPathW returns DWORD 0\n"); + PERF_EXIT(GetTempPathW); + return 0; } - else if ( dwRetVal + 1 > nBufferLength ) + + char TempBuffer[nBufferLength > 0 ? nBufferLength : 1]; + DWORD dwRetVal = GetTempPathA( nBufferLength, TempBuffer ); + + if ( dwRetVal >= nBufferLength ) { - // if dwRetVal < MAX_PATH, dwRetVal = number of TCHARs in TempBuffer, not including NULL ERROR( "lpBuffer was not large enough.\n" ) SetLastError( ERROR_INSUFFICIENT_BUFFER ); *lpBuffer = '\0'; diff --git a/src/pal/tests/palsuite/file_io/GetTempPathW/test1/GetTempPathW.c b/src/pal/tests/palsuite/file_io/GetTempPathW/test1/GetTempPathW.c index f0f0203..75e9699 100644 --- a/src/pal/tests/palsuite/file_io/GetTempPathW/test1/GetTempPathW.c +++ b/src/pal/tests/palsuite/file_io/GetTempPathW/test1/GetTempPathW.c @@ -14,128 +14,91 @@ #include -static void SetEnvVar(WCHAR tmpValue[], WCHAR tempPath[]) +static void SetTmpDir(WCHAR path[]) { - BOOL checkValue=FALSE; - - /* see if the environment variable is created correctly */ - checkValue = SetEnvironmentVariableW(tmpValue, tempPath); - if(!checkValue) + DWORD result = SetEnvironmentVariableW(W("TMPDIR"), path); + if (!result) { - if (tempPath == NULL) - { - if (SetEnvironmentVariableW(tmpValue, convert("")) && - SetEnvironmentVariableW(tmpValue, NULL)) - { - return; - } - } - - Fail("GetTempPathA: ERROR -> Failed to set the environment " - "variable correctly.\n"); - } + Fail("ERROR -> SetEnvironmentVariableW failed with result %d and error code %d.\n", + result, GetLastError()); + } } -int __cdecl main(int argc, char *argv[]) +static void SetAndCompare(WCHAR tmpDirPath[], WCHAR expected[]) { - DWORD dwBuffLength = _MAX_DIR; - WCHAR wPath[_MAX_DIR]; - WCHAR tmpValue[] = {'T','M','P','\0'}; - WCHAR tempValue[] = {'T','E','M','P','\0'}; -#if WIN32 - WCHAR tempPath[] = {'C',':','\\','t','e','m','p','\\','\0'}; - WCHAR tmpPath[] = {'C',':','\\','t','m','p','\\','\0'}; -#else - WCHAR tempPath[] = {'.','\\','t','e','m','p','\\','\0'}; - WCHAR tmpPath[] = {'.','\\','t','m','p','\\','\0'}; -#endif + DWORD dwBufferLength = _MAX_DIR; + WCHAR path[dwBufferLength]; + SetTmpDir(tmpDirPath); - if (0 != PAL_Initialize(argc,argv)) + DWORD dwResultLen = GetTempPathW(dwBufferLength, path); + if (dwResultLen <= 0) { - return FAIL; + Fail("ERROR: GetTempPathW returned %d with error code %d.\n", dwResultLen, GetLastError()); } - - if (GetTempPathW(dwBuffLength, wPath) == 0) + if (dwResultLen >= dwBufferLength) { - Fail("GetTempPathW: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); + Fail("ERROR: Buffer of length %d passed to GetTempPathA was too small to hold %d chars..\n", dwBufferLength, dwResultLen); } - else + if (wcscmp(expected, path) != 0) { - // CreateDirectory should fail on an existing directory - if (CreateDirectoryW(wPath, NULL) != 0) - { - Fail("GetTempPathW: ERROR -> The path returned is apparently " - "invalid since CreateDirectory succeeded whereas it should " - "have failed.\n"); - } + Fail("ERROR: GetTempPathW expected to get '%S' but instead got '%S'.\n", expected, path); } - - /* set both tmp and temp to null and check that gettemppathw returns - a non zero value*/ - SetEnvVar(tmpValue, NULL); - - /* create the environment variable */ - SetEnvVar(tempValue, tempPath); - - /* set the environment variable to NULL */ - SetEnvVar(tempValue, NULL); - - if(GetTempPathW(dwBuffLength, wPath) == 0) + if (expected[dwResultLen - 1] != '/') { - Fail("GetTempPathW: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); + Fail("ERROR: GetTempPathW returned '%S', which should have ended in '/'.\n", path); } +} - /* set temp, and gettemppathw should return value of temp */ - SetEnvVar(tempValue, tempPath); - - if(GetTempPathW(dwBuffLength, wPath) == 0) - { - Fail("GetTempPathW: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); - } - - if(wcscmp(wPath, tempPath) != 0) - { - Fail("GetTempPathW: ERROR -> Failed to return correct temporary path. " - "Expected path %s but got %s.\n", tempPath, wPath); - } +static void SetAndCheckLength(WCHAR tmpDirPath [], int bufferLength, int expectedResultLength) +{ + WCHAR path[bufferLength]; - /* set temp to null, and set temp to a proper value, - gettemppathw should return value stored in tmp */ - SetEnvVar(tempValue, NULL); - SetEnvVar(tmpValue, tmpPath); - - if(GetTempPathW(dwBuffLength, wPath) == 0) - { - Fail("GetTempPathW: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); - } - - if(wcscmp(wPath, tmpPath) != 0) + SetTmpDir(tmpDirPath); + DWORD dwResultLen = GetTempPathW(bufferLength, path); + + if (dwResultLen != expectedResultLength) { - Fail("GetTempPathW: ERROR -> Failed to return correct temporary path. " - "Expected path %s but got %s.\n", tmpPath, wPath); + Fail("GetTempPathW(%d, %S) expected to return %d but returned %d.\n", + bufferLength, tmpDirPath?tmpDirPath:W("NULL"), expectedResultLength, dwResultLen); } +} - /* set temp and gettemppathw should return value stored in tmp */ - SetEnvVar(tempValue, tempPath); - - if(GetTempPathW(dwBuffLength, wPath) == 0) +int __cdecl main(int argc, char *argv[]) +{ + if (0 != PAL_Initialize(argc, argv)) { - Fail("GetTempPathW: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); + return FAIL; } - - if(wcscmp(wPath, tmpPath) != 0) + + SetAndCompare(W("/tmp"), W("/tmp/")); + SetAndCompare(W("/tmp/"), W("/tmp/")); + SetAndCompare(W(""), W("/tmp/")); + SetAndCompare(NULL, W("/tmp/")); + SetAndCompare(W("/"), W("/")); + SetAndCompare(W("/var/tmp"), W("/var/tmp/")); + SetAndCompare(W("/var/tmp/"), W("/var/tmp/")); + SetAndCompare(W("~"), W("~/")); + SetAndCompare(W("~/"), W("~/")); + SetAndCompare(W(".tmp"), W(".tmp/")); + SetAndCompare(W("./tmp"), W("./tmp/")); + SetAndCompare(W("/home/someuser/sometempdir"), W("/home/someuser/sometempdir/")); + SetAndCompare(NULL, W("/tmp/")); + + DWORD dwResultLen = GetTempPathA(0, NULL); + if (dwResultLen != 0 || GetLastError() != ERROR_INVALID_PARAMETER) { - Fail("GetTempPathW: ERROR -> Failed to return correct temporary path. " - "Expected path %s but got %s.\n", tmpPath, wPath); + Fail("GetTempPathW(NULL, ...) returned %d with error code %d but " + "should have failed with ERROR_INVALID_PARAMETER (%d).\n", + dwResultLen, GetLastError(), ERROR_INVALID_PARAMETER); } + SetAndCheckLength(W("abc/"), 5, 4); + SetAndCheckLength(W("abcd"), 5, 6); + SetAndCheckLength(W("abcde"), 5, 7); + SetAndCheckLength(W("abcdef/"), 5, 9); + SetAndCheckLength(NULL, 5, 6); + PAL_Terminate(); return PASS; } - diff --git a/src/pal/tests/palsuite/file_io/gettemppatha/test1/gettemppatha.c b/src/pal/tests/palsuite/file_io/gettemppatha/test1/gettemppatha.c index 4a60084..b3c428f 100644 --- a/src/pal/tests/palsuite/file_io/gettemppatha/test1/gettemppatha.c +++ b/src/pal/tests/palsuite/file_io/gettemppatha/test1/gettemppatha.c @@ -14,133 +14,91 @@ #include - -static void SetEnvVar(char tmpValue[], char tempPath[]) +static void SetTmpDir(CHAR path[]) { - BOOL checkValue=FALSE; - - /* see if the environment variable is created correctly */ - checkValue = SetEnvironmentVariable(tmpValue, tempPath); - if(!checkValue) + DWORD result = SetEnvironmentVariableA("TMPDIR", path); + if (!result) { - if (tempPath == NULL) - { - if (SetEnvironmentVariable(tmpValue, "") && - SetEnvironmentVariable(tmpValue, NULL)) - { - return; - } - } - - Fail("GetTempPathA: ERROR -> Failed to set the environment " - "variable correctly.\n"); - } + Fail("ERROR -> SetEnvironmentVariableA failed with result %d and error code %d.\n", + result, GetLastError()); + } } - -int __cdecl main(int argc, char *argv[]) +static void SetAndCompare(CHAR tmpDirPath[], CHAR expected[]) { - DWORD dwBuffLength = _MAX_DIR; - CHAR path[_MAX_DIR]; - char* tmpValue = "TMP"; - char* tempValue = "TEMP"; -#if WIN32 - char tempPath[] = "C:\\temp\\"; - char tmpPath[] = "C:\\tmp\\"; -#else - char tempPath[] = ".\\temp\\"; - char tmpPath[] = ".\\tmp\\"; -#endif - + DWORD dwBufferLength = _MAX_DIR; + CHAR path[dwBufferLength]; + + SetTmpDir(tmpDirPath); - if (0 != PAL_Initialize(argc,argv)) + DWORD dwResultLen = GetTempPathA(dwBufferLength, path); + if (dwResultLen <= 0) { - return FAIL; + Fail("ERROR: GetTempPathA returned %d with error code %d.\n", dwResultLen, GetLastError()); } - - if (GetTempPathA(dwBuffLength, path) == 0) + if (dwResultLen >= dwBufferLength) { - Fail("GetTempPathA: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); + Fail("ERROR: Buffer of length %d passed to GetTempPathA was too small to hold %d chars..\n", dwBufferLength, dwResultLen); } - else + if (strcmp(expected, path) != 0) { - // CreateDirectory should fail on an existing directory - if (CreateDirectoryA(path, NULL) != 0) - { - Fail("GetTempPathA: ERROR -> The path returned is apparently " - "invalid since CreateDirectory succeeded whereas it should " - "have failed.\n"); - } + Fail("ERROR: GetTempPathA expected to get '%s' but instead got '%s'.\n", expected, path); } - - - /* set both tmp and temp to null and check that gettemppath returns - a non zero value*/ - SetEnvVar(tmpValue, NULL); - - /* create the variable tempValue */ - SetEnvVar(tempValue, ""); - - /* set tempValue to null initially */ - SetEnvironmentVariable(tempValue, NULL); - - /* get the temp path */ - if(GetTempPathA(dwBuffLength, path) == 0) + if (expected[dwResultLen - 1] != '/') { - Fail("GetTempPathA: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); + Fail("ERROR: GetTempPathA returned '%s', which should have ended in '/'.\n", path); } +} - /* set temp, and gettemppatha should return value of temp */ - SetEnvVar(tempValue, tempPath); +static void SetAndCheckLength(CHAR tmpDirPath[], int bufferLength, int expectedResultLength) +{ + CHAR path[bufferLength]; - if(GetTempPathA(dwBuffLength, path) == 0) - { - Fail("GetTempPathA: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); - } - - if(strcmp(path,tempPath) != 0) - { - Fail("GetTempPathA: ERROR -> Failed to return correct temporary path. " - "Expected path %s but got %s.\n", tempPath, path); - } + SetTmpDir(tmpDirPath); + DWORD dwResultLen = GetTempPathA(bufferLength, path); - /* set temp to null, and set temp to a proper value, - gettemppatha should return value stored in tmp */ - SetEnvVar(tempValue, NULL); - SetEnvVar(tmpValue, tmpPath); - - if(GetTempPathA(dwBuffLength, path) == 0) + if (dwResultLen != expectedResultLength) { - Fail("GetTempPathA: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); + Fail("GetTempPathA(%d, %s) expected to return %d but returned %d.\n", + bufferLength, tmpDirPath?tmpDirPath:"NULL", expectedResultLength, dwResultLen); } +} - if(strcmp(path,tmpPath) != 0) +int __cdecl main(int argc, char *argv[]) +{ + if (0 != PAL_Initialize(argc,argv)) { - Fail("GetTempPathA: ERROR -> Failed to return correct temporary path. " - "Expected path %s but got %s.\n", tmpPath, path); + return FAIL; } - /* set temp and gettemppatha should return value stored in tmp */ - SetEnvVar(tempValue, tempPath); - - if(GetTempPathA(dwBuffLength, path) == 0) + SetAndCompare("/tmp", "/tmp/"); + SetAndCompare("/tmp/", "/tmp/"); + SetAndCompare("", "/tmp/"); + SetAndCompare(NULL, "/tmp/"); + SetAndCompare("/", "/"); + SetAndCompare("/var/tmp", "/var/tmp/"); + SetAndCompare("/var/tmp/", "/var/tmp/"); + SetAndCompare("~", "~/"); + SetAndCompare("~/", "~/"); + SetAndCompare(".tmp", ".tmp/"); + SetAndCompare("./tmp", "./tmp/"); + SetAndCompare("/home/someuser/sometempdir", "/home/someuser/sometempdir/"); + SetAndCompare(NULL, "/tmp/"); + + DWORD dwResultLen = GetTempPathA(0, NULL); + if (dwResultLen != 0 || GetLastError() != ERROR_INVALID_PARAMETER) { - Fail("GetTempPathA: ERROR -> Failed to return a temporary path. " - "error code: %ld\n", GetLastError()); + Fail("GetTempPath(NULL, ...) returned %d with error code %d but " + "should have failed with ERROR_INVALID_PARAMETER (%d).\n", + dwResultLen, GetLastError(), ERROR_INVALID_PARAMETER); } - if(strcmp(path,tmpPath) != 0) - { - Fail("GetTempPathA: ERROR -> Failed to return correct temporary path. " - "Expected path %s but got %s.\n", tmpPath, path); - } - + SetAndCheckLength("abc/", 5, 4); + SetAndCheckLength("abcd", 5, 6); + SetAndCheckLength("abcde", 5, 7); + SetAndCheckLength("abcdef/", 5, 9); + SetAndCheckLength(NULL, 5, 6); + PAL_Terminate(); return PASS; } - - diff --git a/src/pal/tests/palsuite/paltestlist.txt b/src/pal/tests/palsuite/paltestlist.txt index 051d858..c7f2127 100644 --- a/src/pal/tests/palsuite/paltestlist.txt +++ b/src/pal/tests/palsuite/paltestlist.txt @@ -577,6 +577,8 @@ file_io/GetTempFileNameA/test1/paltest_gettempfilenamea_test1 file_io/GetTempFileNameA/test2/paltest_gettempfilenamea_test2 file_io/GetTempFileNameA/test3/paltest_gettempfilenamea_test3 file_io/GetTempFileNameW/test3/paltest_gettempfilenamew_test3 +file_io/gettemppatha/test1/paltest_gettemppatha_test1 +file_io/GetTempPathW/test1/paltest_gettemppathw_test1 file_io/ReadFile/test2/paltest_readfile_test2 file_io/ReadFile/test3/paltest_readfile_test3 file_io/ReadFile/test4/paltest_readfile_test4 -- 2.7.4