Change the default --tmp-dir path to a more suitable path

In commit 4e1cc5f6dda22e9 the create_temp_filename() function was reviewed and hardened, which in the end renamed this function to create_temp_file() in commit 495e3cec5d156.

With these changes it became more evident that OpenVPN needs a directory where it can create temporary files.  The create_temp_file() will create such files f.ex. if --client-connect or --plugin which makes use of the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY hook, such as openvpn-auth-pam.so.

When this happens, OpenVPN will normally create these files in the directory OpenVPN was started.  In many cases, this will fail due to restricted access. By using --tmp-dir and pointing it to a directory writeable to the user running OpenVPN, it works again.

This patch makes OpenVPN use a more suitable temproary directory by default, instead of the current working directory.  On non-Windows platforms this default value is set to '/tmp', but can be modified at compile-time by running ./configure --with-tmp-dir-path=<TEMP DIR PATH>.  On Windows, it will look up %TEMP% and %TMP% first, and if that doesn't give any clues, it will fallback to C:\WINDOWS\Temp in the end.

configure.ac +5 -0

+AC_ARG_WITH(tmp-dir-path,
+   [  --with-tmp-dir-path=PATH  Default tmp-dir to use when not configured (unset defaults to /tmp)],
+   [TMPDIRPATH="$withval"]
+)
+

Quoted 14 lines · David Sommerseth

In commit 4e1cc5f6dda22e9 the create_temp_filename() function was reviewed and hardened, which in the end renamed this function to create_temp_file() in commit 495e3cec5d156.

With these changes it became more evident that OpenVPN needs a directory where it can create temporary files. The create_temp_file() will create such files f.ex. if --client-connect or --plugin which makes use of the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY hook, such as openvpn-auth-pam.so.

When this happens, OpenVPN will normally create these files in the directory OpenVPN was started. In many cases, this will fail due to restricted access. By using --tmp-dir and pointing it to a directory writeable to the user running OpenVPN, it works again.

Quoted 6 lines

This patch makes OpenVPN use a more suitable temproary directory by default, instead of the current working directory. On non-Windows platforms this default value is set to '/tmp', but can be modified at compile-time by running ./configure --with-tmp-dir-path=<TEMP DIR PATH>. On Windows, it will look up %TEMP% and %TMP% first, and if that doesn't give any clues, it will fallback to C:\WINDOWS\Temp in the end.

I don't understand,
if you use windows environment variables, then why not do the same on Unix? You have the standard TMPDIR [1] variable, and fallback to /tmp.

Also, at Windows you should go into %SystemRoot%\Temp using ExpandEnvironmentVariable() and not hardcode C:\

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

[resend copy to openvpn-devel list as well]

Quoted 23 lines · Alon Bar-Lev

Quoted 14 lines · David Sommerseth

In commit 4e1cc5f6dda22e9 the create_temp_filename() function was reviewed and hardened, which in the end renamed this function to create_temp_file() in commit 495e3cec5d156.

With these changes it became more evident that OpenVPN needs a directory where it can create temporary files. The create_temp_file() will create such files f.ex. if --client-connect or --plugin which makes use of the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY hook, such as openvpn-auth-pam.so.

When this happens, OpenVPN will normally create these files in the directory OpenVPN was started. In many cases, this will fail due to restricted access. By using --tmp-dir and pointing it to a directory writeable to the user running OpenVPN, it works again.

Quoted 7 lines

This patch makes OpenVPN use a more suitable temproary directory by default, instead of the current working directory. On non-Windows platforms this default value is set to '/tmp', but can be modified at compile-time by running ./configure --with-tmp-dir-path=<TEMP DIR PATH>. On Windows, it will look up %TEMP% and %TMP% first, and if that doesn't give any clues, it will fallback to C:\WINDOWS\Temp in the end.

Quoted 4 lines

I don't understand,
if you use windows environment variables, then why not do the same on Unix? You have the standard TMPDIR [1] variable, and fallback to /tmp.

I checked for the $TMPDIR variable on CentOS 5.5, Fedora 14 and Gentoo installations. And $TMPDIR didn't show up at all, hence I thought this was not a really useful option. However, I see from the wikipage that this is supposed to be part of SuS. But it seems not to be respected in Linux at least. But fair point. I can add a similar logic to non-Windows installations as well, again with a hard-coded fallback.

Quoted 3 lines

Also, at Windows you should go into %SystemRoot%\Temp using ExpandEnvironmentVariable() and not hardcode C:\

Good idea! I wasn't aware of that one. I'll fix this. I will anyway choose to fallback to C:\WINDOWS\Temp if %SystemRoot% is not found, even though I believe this is most likely not something which should happen.

I'll implement the suggested change for autotools as well and propose an additional patch to cover your comments.

Quoted 56 lines · David Sommerseth

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

[resend copy to openvpn-devel list as well]

Quoted 28 lines · Alon Bar-Lev

Quoted 17 lines · David Sommerseth

In commit 4e1cc5f6dda22e9 the create_temp_filename() function was reviewed and hardened, which in the end renamed this function to create_temp_file() in commit 495e3cec5d156.

With these changes it became more evident that OpenVPN needs a directory where it can create temporary files. The create_temp_file() will create such files f.ex. if --client-connect or --plugin which makes use of the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY hook, such as openvpn-auth-pam.so.

When this happens, OpenVPN will normally create these files in the directory
OpenVPN was started. In many cases, this will fail due to restricted access.
By using --tmp-dir and pointing it to a directory writeable to the user running OpenVPN, it works again.

Quoted 9 lines

This patch makes OpenVPN use a more suitable temproary directory by default,
instead of the current working directory. On non-Windows platforms this default value is set to '/tmp', but can be modified at compile-time by running ./configure --with-tmp-dir-path=<TEMP DIR PATH>. On Windows, it will look up %TEMP% and %TMP% first, and if that doesn't give any clues, it
will fallback to C:\WINDOWS\Temp in the end.

Quoted 4 lines

I don't understand,
if you use windows environment variables, then why not do the same on Unix? You have the standard TMPDIR [1] variable, and fallback to /tmp.

I checked for the $TMPDIR variable on CentOS 5.5, Fedora 14 and Gentoo installations. And $TMPDIR didn't show up at all, hence I thought this was not a really useful option. However, I see from the wikipage that this is supposed to be part of SuS. But it seems not to be respected in Linux at least. But fair point. I can add a similar logic to non-Windows installations as well, again with a hard-coded fallback.

Quoted 3 lines

Also, at Windows you should go into %SystemRoot%\Temp using ExpandEnvironmentVariable() and not hardcode C:\

Good idea! I wasn't aware of that one. I'll fix this. I will anyway choose to fallback to C:\WINDOWS\Temp if %SystemRoot% is not found, even though I believe this is most likely not something which should happen.

Quoted 4 lines

I'll implement the suggested change for autotools as well and propose an additional patch to cover your comments.

err , didn't we agree to use %TEMP% on windows? AFAIK this env var is always there...

And yes, on my Linux boxen there is no $TMPDIR, but I'd like to be able to overrule the temporary directory anyways....
So as far as I am concerned the Linux version of the patch is perfect.

[resend copy to openvpn-devel list as well]

I checked for the $TMPDIR variable on CentOS 5.5, Fedora 14 and Gentoo
installations. And $TMPDIR didn't show up at all, hence I thought this was
not a really useful option. However, I see from the wikipage that this is
supposed to be part of SuS. But it seems not to be respected in Linux
at
least.

FYI After poking about the man pages of various Un*xs I find:

login(1) does not have to set $TMPDIR, although the whole login process can be configured to do so there's no requirement that $TMPDIR exist.

Please stop trying to find problems where not exist.

Good idea!  I wasn't aware of that one.  I'll fix this.  I will anyway choose to fallback to C:\WINDOWS\Temp if %SystemRoot% is not found, even though I believe this is most likely not something which should happen.

I'll implement the suggested change for autotools as well and propose an additional patch to cover your comments.

I really think you should remove this from compile time (autoconf). It is useless, users can override it anyway in configuration, so there is no need for this.

Quoted 3 lines · David Sommerseth

Good idea! I wasn't aware of that one. I'll fix this. I will anyway choose to fallback to C:\WINDOWS\Temp if %SystemRoot% is not found, even though I believe this is most likely not something which should happen.

Can't happen, Windows will not boot.

According to the Single Unix Specification (SuS) [1] the $TMPDIR environment variable may define another temporary directory than the standard /tmp directory. As it is considered safe to assume /tmp will always exist, remove the possibility to change this at compile time.

It is expected that %SystemRoot% is always defined to the directory of the Windows system. If this is undefined, Windows do not work properly.

So instead of hard-coding C:\WINDOWS\Temp as the default temporary directory if %TEMP% or %TMP% is not found, use %SystemRoot%\Temp instead as the fallback.

+ return NULL; /* Just in case M_FATAL doesn't exit() */

win32.c +14 -0

+
+  envptr = getenv(SYS_PATH_ENV_VAR_NAME);
+  if( !envptr ) {
+    /* This indicates something is really wrong with the Windows
+     * environment, and we shouldn't try to start OpenVPN in this case
+     */
+    msg (M_FATAL, "Could not find %%%s%% environment variable",
+         SYS_PATH_ENV_VAR_NAME);
+    return NULL; /* Just in case M_FATAL doesn't exit() */
+  }
+
+  openvpn_snprintf(tmpdir, 256, "%.249s\\Temp", envptr);
+  msg (M_WARN, "Could not find %%TEMP%% or %%TMP%% environment variables.  "
+       "Falling back to %s.  Consider to use --tmp-dir", tmpdir);

  return tmpdir;
 }

win32.c +2 -4

-  /* Try to use %TEMP% or %TMP% */
-  tmpdir = getenv("TEMP");
-  if( !tmpdir ) {
-    tmpdir = getenv("TMP");
+  CLEAR (tmpdir);
+

Why not a bit simpler; GetTempPath() (kernel32.dll) already does these tests.
Ref: http://msdn.microsoft.com/en-us/library/aa364992(v=vs.85).aspx :

Quoted 6 lines · David Sommerseth

win32.c +2 -4

-  /* Try to use %TEMP% or %TMP% */
-  tmpdir = getenv("TEMP");
-  if( !tmpdir ) {
-    tmpdir = getenv("TMP");
+  CLEAR (tmpdir);
+

win32.c env_set_add_win32 (struct env_set *es) · line 1111

Quoted 10 lines

win32.c +4 -5

+  /* Try to use %TEMP%, %TMP% or %SystemRoot%\Temp */
+  envptr = getenv("TEMP");
+  if( envptr ) {
+    return envptr;
  }
-  if( !tmpdir ) {
-    /* Warn if we're using a hard coded path */
-    msg (M_WARN, "Could not find %TEMP% or %TMP% environment variables.  "
-         "Falling back to %s.  Consider to use --tmp-dir", DEFAULT_TMPDIR);
-    tmpdir = DEFAULT_TMPDIR;

Why not a bit simpler; GetTempPath() (kernel32.dll) already does these tests.
Ref: http://msdn.microsoft.com/en-us/library/aa364992(v=vs.85).aspx :

The GetTempPath function checks for the existence of environment variables in the following order and uses the first path found:
1.. The path specified by the TMP environment variable. 2.. The path specified by the TEMP environment variable. 3.. The path specified by the USERPROFILE environment variable. 4.. The Windows directory.

On Windows, it will look up %TEMP% and %TMP% first, and if that doesn't give any clues, it will fallback to C:\WINDOWS\Temp in the end.