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.

4 files changed, 195 insertions, 234 deletions

diff --git a/src/pal/src/file/path.cpp b/src/pal/src/file/path.cpp
index 81ee86abff..e85104fb77 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 f0f020323e..75e9699dcf 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 <palsuite.h>
 
-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 4a6008427f..b3c428fdea 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 <palsuite.h>
 
-
-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 051d858321..c7f2127415 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