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

PATCH Applied David Sommerseth 4 participants 6 messages Apr 14, 2011 – Apr 15, 2011

Share & tools

David Sommerseth

author

2011-04-14 14:29 UTC #0 lore raw

PATCH v2 1/1 Change the default --tmp-dir path to a more suitable path

After all the discussions regarding the --tmp-dir patch [1], I have now condenced everything into one single patch. The main change is that the new win_get_tempdir() function is simplified by using GetTempPath() instead.

On Windows the fallback solution, if GetTempPath() returns NULL, is now to behave as before - write temporary files in the directory where OpenVPN was started.

[1] <http://thread.gmane.org/gmane.network.openvpn.devel/4561>

David Sommerseth (1):
Change the default --tmp-dir path to a more suitable path

 options.c |   18 ++++++++++++++----
 win32.c   |   19 +++++++++++++++++++
 win32.h   |    3 +++
 3 files changed, 36 insertions(+), 4 deletions(-)

Signature · 1 line

1.7.4.2

David Sommerseth

author

2011-04-14 14:29 UTC #1 lore raw

PATCH v2 1/1 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 use GetTempPath() to find temporary paths recommended by the OS. If this fails, it will fallback to the old behaviour, using the directory where OpenVPN was started.

In any cases, this default value can be overridden in the configuration file by using the --tmp-dir option, as before.

To check what the default is at runime, you can see this easily by doing this:

      $ ./openvpn --verb 4 --dev tun | grep tmp_dir

Signed-off-by David Sommerseth <[email protected]...>

Tested-by Jan Just Keijser <[email protected]...>

---
 options.c |   18 ++++++++++++++----
 win32.c   |   19 +++++++++++++++++++
 win32.h   |    3 +++
 3 files changed, 36 insertions(+), 4 deletions(-)

options.c +14 -4

index 36e8393..7303cb4 100644
@@ -766,11 +766,23 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_X509ALTUSERNAME
   o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
-#endif
-#endif
+#endif /* USE_SSL */
+#endif /* USE_CRYPTO */
 #ifdef ENABLE_PKCS11
   o->pkcs11_pin_cache_period = -1;
 #endif			/* ENABLE_PKCS11 */
+
+  /* Set default --tmp-dir */
+#ifdef WIN32
+  /* On Windows, find temp dir via enviroment variables */
+  o->tmp_dir = win_get_tempdir();
+#else
+  /* Non-windows platforms use $TMPDIR, and if not set, default to '/tmp' */
+  o->tmp_dir = getenv("TMPDIR");
+  if( !o->tmp_dir ) {
+          o->tmp_dir = "/tmp";
+  }
+#endif /* WIN32 */
 }
 
 void

@@ -1916,8 +1928,6 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 	msg (M_USAGE, "--client-connect requires --mode server");
       if (options->client_disconnect_script)
 	msg (M_USAGE, "--client-disconnect requires --mode server");
-      if (options->tmp_dir)
-	msg (M_USAGE, "--tmp-dir requires --mode server");
       if (options->client_config_dir || options->ccd_exclusive)
 	msg (M_USAGE, "--client-config-dir/--ccd-exclusive requires --mode server");
       if (options->enable_c2c)

win32.c +19 -0

index 7c9901e..2b7bf7b 100644
@@ -1093,4 +1093,23 @@ env_set_add_win32 (struct env_set *es)
   set_win_sys_path (DEFAULT_WIN_SYS_PATH, es);
 }
 
+
+const char *
+win_get_tempdir()
+{
+  static char buf[MAX_PATH];
+  char *tmpdir = buf;
+
+  CLEAR(buf);
+
+  if (!GetTempPath(sizeof(buf),buf)) {
+    /* Warn if we can't find a valid temporary directory, which should
+     * be unlikely.
+     */
+    msg (M_WARN, "Could not find a suitable temporary directory."
+         " (GetTempPath() failed).  Consider to use --tmp-dir");
+    tmpdir = NULL;
+  }
+  return tmpdir;
+}
 #endif

win32.h +3 -0

index fcc3062..b6a162e 100644
@@ -270,5 +270,8 @@ char *get_win_sys_path (void);
 /* call self in a subprocess */
 void fork_to_self (const char *cmdline);
 
+/* Find temporary directory */
+const char *win_get_tempdir();
+
 #endif
 #endif

Signature · 1 line

1.7.4.2

Gert Doering

2011-04-14 19:03 UTC #2 lore raw

Hi,

Quoted 1 line · David Sommerseth

This patch makes OpenVPN use a more suitable temproary directory by default,

ACK.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Peter Stuge

2011-04-14 21:52 UTC #3 lore raw

In reply to David Sommerseth

Quoted 25 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.
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 use GetTempPath() to find temporary paths recommended by the OS. If this fails, it will fallback to the old behaviour, using the directory where OpenVPN was started.
In any cases, this default value can be overridden in the configuration file by using the --tmp-dir option, as before.

Quoted 7 lines

To check what the default is at runime, you can see this easily by doing this:
      $ ./openvpn --verb 4 --dev tun | grep tmp_dir
Signed-off-by David Sommerseth <[email protected]...>
Tested-by Jan Just Keijser <[email protected]...>

The above commit message doesn't really fit the patch anymore. :)

Quoted 5 lines

options.c +0 -0

index 36e8393..7303cb4 100644

options.c init_options (struct options *o, const bool init_gc) · line 773

Quoted 10 lines

options.c +2 -2

@@ -766,11 +766,23 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_X509ALTUSERNAME
   o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
-#endif
-#endif
+#endif /* USE_SSL */
+#endif /* USE_CRYPTO */
 #ifdef ENABLE_PKCS11
   o->pkcs11_pin_cache_period = -1;
 #endif			/* ENABLE_PKCS11 */

The above hunk is not really related, right? Looks fine otherwise!

//Peter

David Sommerseth

2011-04-15 08:08 UTC #4 lore raw

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

Quoted 29 lines · Peter Stuge

Quoted 28 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.
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 use GetTempPath() to find temporary paths recommended by the OS. If this fails, it will fallback to the old behaviour, using the directory where OpenVPN was started.
In any cases, this default value can be overridden in the configuration file by using the --tmp-dir option, as before.
To check what the default is at runime, you can see this easily by doing this:

Quoted 6 lines

Quoted 4 lines
      $ ./openvpn --verb 4 --dev tun | grep tmp_dir
Signed-off-by David Sommerseth <[email protected]...>
Tested-by Jan Just Keijser <[email protected]...>
The above commit message doesn't really fit the patch anymore. :)

Gah ... yeah, I see now that the compile-time stuff should be removed as well. That slipped through. I'll push a git note to that commit. The patch is already applied, I'd rather avoid redoing the history.

Quoted 9 lines

options.c +0 -1

index 36e8393..7303cb4 100644
@@ -766,11 +766,23 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_X509ALTUSERNAME
   o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
-#endif

Quoted 8 lines

options.c init_options (struct options *o, const bool init_gc) · line 773
Quoted 6 lines
options.c +2 -1
-#endif
+#endif /* USE_SSL */
+#endif /* USE_CRYPTO */
 #ifdef ENABLE_PKCS11
   o->pkcs11_pin_cache_period = -1;
 #endif			/* ENABLE_PKCS11 */
The above hunk is not really related, right? Looks fine otherwise!

I left that part on purpose, as it is completely harmless, code-wise. I had to step carefully in the #ifdef nesting when adding my changes. So I figured adding helpful comments in the same region where the code is modified is beneficial after all. I could have mentioned it in the commit log though.

Gert already commented it on irc to me before giving it an ACK, so it's been spotted already and we let it pass.

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2n/RYACgkQDC186MBRfroWXwCePAT57lEORmXhfyWK1MaCe13B e0oAn0B2GGZaJSiMAINQdRz2xbRFXS6Y
=AInB
-----END PGP SIGNATURE-----

David Sommerseth

Applied

2011-04-15 08:34 UTC #5 lore raw

In reply to David Sommerseth

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

Quoted 13 lines · David Sommerseth

After all the discussions regarding the --tmp-dir patch [1], I have now condenced everything into one single patch. The main change is that the new win_get_tempdir() function is simplified by using GetTempPath() instead.
On Windows the fallback solution, if GetTempPath() returns NULL, is now to behave as before - write temporary files in the directory where OpenVPN was started.
[1] <http://thread.gmane.org/gmane.network.openvpn.devel/4561>

Quoted 7 lines

David Sommerseth (1):
Change the default --tmp-dir path to a more suitable path
 options.c |   18 ++++++++++++++----
 win32.c   |   19 +++++++++++++++++++
 win32.h   |    3 +++
 3 files changed, 36 insertions(+), 4 deletions(-)

Applied to master and beta2.2.

commit eb4b1bb6adc7fb1828839967a7807b6317305145 (beta2.2) commit eb4b1bb6adc7fb1828839967a7807b6317305145 (master)

Author: David Sommerseth <[email protected]...>
Date:   Thu Apr 14 16:21:16 2011 +0200

    Signed-off-by: David Sommerseth <[email protected]...>
    Tested-by: Jan Just Keijser <[email protected]...>
    Acked-by: Gert Doering <[email protected]...>

Kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2oAyUACgkQDC186MBRfroWKgCdFp47roG5t8GlwBUtbqSWl36I N1AAn2uPVZgh8itgrQlLUi/hZcfrUEOz
=t8A0
-----END PGP SIGNATURE-----