Menu
▾
▴
openvpn-devel
|
From: David S. <da...@re...> - 2011-04-06 16:10:19
|
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.
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 <da...@re...>
Tested-by: Jan Just Keijser <ja...@ni...>
---
configure.ac | 11 +++++++++++
options.c | 15 +++++++++++----
win/config.h.in | 3 +++
win32.c | 19 +++++++++++++++++++
win32.h | 3 +++
5 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac
index e0847bc..a9f022e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -279,6 +279,11 @@ AC_ARG_WITH(netstat-path,
)
AC_DEFINE_UNQUOTED(NETSTAT_PATH, "$NETSTAT", [Path to netstat tool])
+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"]
+)
+
AC_ARG_WITH(mem-check,
[ --with-mem-check=TYPE Build with debug memory checking, TYPE = dmalloc or valgrind],
[MEMCHECK="$withval"]
@@ -566,6 +571,12 @@ LDFLAGS="$LDFLAGS -Wl,--fatal-warnings"
AC_CHECK_FUNC(epoll_create, AC_DEFINE(HAVE_EPOLL_CREATE, 1, [epoll_create function is defined]))
LDFLAGS="$OLDLDFLAGS"
+dnl set the default temporary directory
+if test -z "$TMPDIRPATH"; then
+ TMPDIRPATH="/tmp"
+fi
+AC_DEFINE_UNQUOTED(DEFAULT_TMPDIR, "$TMPDIRPATH", [Default --tmp-dir])
+
dnl
dnl check for valgrind tool
dnl
diff --git a/options.c b/options.c
index 36e8393..0b91657 100644
--- a/options.c
+++ b/options.c
@@ -766,11 +766,20 @@ 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 have this value defined via ./configure */
+ o->tmp_dir = DEFAULT_TMPDIR;
+#endif /* WIN32 */
}
void
@@ -1916,8 +1925,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)
diff --git a/win/config.h.in b/win/config.h.in
index fb349d0..abf71d2 100644
--- a/win/config.h.in
+++ b/win/config.h.in
@@ -286,6 +286,9 @@ typedef unsigned long in_addr_t;
/* Route command */
#define ROUTE_PATH "route"
+/* Default temporary directory, if not found via getenv() */
+#define DEFAULT_TMPDIR "C:\\WINDOWS\\Temp" /* FIXME: Should be configurable */
+
#ifdef _MSC_VER
/* MSVC++ hacks */
#pragma warning(disable:4244) // conversion from 'foo' to 'bar', possible loss of data
diff --git a/win32.c b/win32.c
index 7c9901e..c9eb184 100644
--- a/win32.c
+++ b/win32.c
@@ -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 *tmpdir = NULL;
+
+ /* Try to use %TEMP% or %TMP% */
+ tmpdir = getenv("TEMP");
+ if( !tmpdir ) {
+ tmpdir = getenv("TMP");
+ }
+ 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;
+ }
+ return tmpdir;
+}
#endif
diff --git a/win32.h b/win32.h
index fcc3062..b6a162e 100644
--- a/win32.h
+++ b/win32.h
@@ -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
--
1.7.4
|
|
From: Alon Bar-L. <alo...@gm...> - 2011-04-07 12:15:29
|
On Wed, Apr 6, 2011 at 7:10 PM, David Sommerseth <da...@re...> wrote: > 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. 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:\ And if you have a sane fallback ot system location, why you need to add a compile time option to override? [1] http://en.wikipedia.org/wiki/TMPDIR > +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"] > +) > + <snip> > +dnl set the default temporary directory > +if test -z "$TMPDIRPATH"; then > + TMPDIRPATH="/tmp" > +fi > +AC_DEFINE_UNQUOTED(DEFAULT_TMPDIR, "$TMPDIRPATH", [Default --tmp-dir]) > + Both should be something like: --- AC_ARG_WITH( [tmp-dir-path], [AS_HELP_STRING([--with-tmp-dir-path=PATH], [Default tmp-dir to use when not configured (unset defaults to /tmp)])], [TMPDIRPATH="$withval"], [ case "${host}" in *-mingw*) TMPDIRPATH="%SystemRoot%/Temp" ;; *) TMPDIRPATH="/tmp" ;; esac ) AC_DEFINE_UNQUOTED([DEFAULT_TMPDIR], [${TMPDIRPATH}], [Default --tmp-dir]) --- Alon. |
|
From: David S. <ope...@to...> - 2011-04-07 12:51:32
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 [resend copy to openvpn-devel list as well] On 07/04/11 14:15, Alon Bar-Lev wrote: > On Wed, Apr 6, 2011 at 7:10 PM, David Sommerseth <da...@re...> wrote: >> 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. > > 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. > 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. Thanks a lot for your review! kind regards, David Sommerseth -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iEYEARECAAYFAk2ds2sACgkQDC186MBRfrqCGwCcDBv5jSlrgSbBG3CsPDFVuehO ME8AnRFDvApIJEmO18inLiw3OoJfFGNW =RKXA -----END PGP SIGNATURE----- |
|
From: Karl O. P. <ko...@me...> - 2011-04-07 14:52:15
|
On 04/07/2011 07:51:55 AM, David Sommerseth wrote:
> [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.
Karl <ko...@me...>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
|
|
From: Alon Bar-L. <alo...@gm...> - 2011-04-07 14:59:57
|
On Thu, Apr 7, 2011 at 5:52 PM, Karl O. Pinc <ko...@me...> wrote: > 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. This is so strange discussion. OKOKOKOKOKOK there is no TMPDIR so you fallback to /tmp What's wrong with that? /tmp exists in POSIX and it is totally valid. If system need to override this it declares the TMPDIR globally. Please stop trying to find problems where not exist. |
|
From: Karl O. P. <ko...@me...> - 2011-04-07 15:14:41
|
On 04/07/2011 09:59:51 AM, Alon Bar-Lev wrote:
> Please stop trying to find problems where not exist.
I think we are on the same page here. I don't see
any sort of problem.
Karl <ko...@me...>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
|
|
From: Jan J. K. <ja...@ni...> - 2011-04-07 12:58:23
|
David Sommerseth wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > [resend copy to openvpn-devel list as well] > > On 07/04/11 14:15, Alon Bar-Lev wrote: > >> On Wed, Apr 6, 2011 at 7:10 PM, David Sommerseth <da...@re...> wrote: >> >>> 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. >>> >> 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. > > >> 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. > > 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. cheers, JJK |
|
From: David S. <ope...@to...> - 2011-04-07 13:07:36
|
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/04/11 14:58, Jan Just Keijser wrote: > David Sommerseth wrote: >> -----BEGIN PGP SIGNED MESSAGE----- >> Hash: SHA1 >> >> [resend copy to openvpn-devel list as well] >> >> On 07/04/11 14:15, Alon Bar-Lev wrote: >> >>> On Wed, Apr 6, 2011 at 7:10 PM, David Sommerseth <da...@re...> wrote: >>> >>>> 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. >>>> >>> 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. >> >> >>> 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. >> >> > > err , didn't we agree to use %TEMP% on windows? AFAIK this env var is > always there... %TEMP% and then %TMP% is checked. Alon's suggestion is to expand the default hardcoded C:\WINDOWS\Temp to use %SystemRoot%\Temp if %TEMP% and %TMP% fails. I like that approach, and will implement that, with C:\WINDOWS\Temp as the final fallback if %SystemRoot% fails. > 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. Good! I'll implement $TMPDIR anyway, just to have that covered, which is more inline with SuS anyway [1]. Fallback will be as it is now anyway. kind regards, David Sommerseth [1] <http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03> -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iEYEARECAAYFAk2dty0ACgkQDC186MBRfroF6gCbB+Xoqu7sqYYLBDpsytH6umnD GoEAn2hjJR5kqpTLDUsAbrS4dJl5yPs6 =yEiA -----END PGP SIGNATURE----- |
|
From: Alon Bar-L. <alo...@gm...> - 2011-04-07 14:04:49
|
On Thu, Apr 7, 2011 at 3:48 PM, David Sommerseth <da...@re...> wrote: > 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. > 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. |
|
From: Peter S. <pe...@st...> - 2011-04-07 14:38:37
|
Alon Bar-Lev wrote: > 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. And users can also set the environment variable to point to where they want. Agree, please remove from autoconf. //Peter |
|
From: Gisle V. <gv...@br...> - 2011-04-07 14:50:52
|
"Alon Bar-Lev" <alo...@gm...> wrote: > On Thu, Apr 7, 2011 at 3:48 PM, David Sommerseth <da...@re...> wrote: >> 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. But a user can still do "set SystemRoot=" after boot. But a lot of problems will arise though. A quick test; notepad.exe doesn't start for instance. --gv |
|
From: David S. <da...@re...> - 2011-04-08 11:39:51
|
These patches goes on-top of the previous --tmp-dir patch [1]. In summary, these patches implement the consensus of the following discussion. * Remove compile time configuration option for default temp dir. In non-Windows environments it is most likely that /tmp will exist anyway. For Windows environments it is now using %SystemRoot%\Temp as the last fallback possibility. If this fails, it is most likely that there are more sever issues which is not related to OpenVPN at all. * Check for $TMPDIR In non-Windows environments, the $TMPDIR directory may be used to set a specific temporary directory path. If this is not found, /tmp will be used as the default. Nothing has changed in this regard for Windows environments, where %TEMP% and %TMP% are checked before the fallback. Kind regards, David Sommerseth [1] <http://thread.gmane.org/gmane.network.openvpn.devel/4561> David Sommerseth (2): Make use of $TMPDIR on non-Windows Use %SystemRoot% instead of hard-coded C:\WINDOWS for temp directory path configure.ac | 11 ----------- options.c | 7 +++++-- win/config.h.in | 3 --- win32.c | 36 ++++++++++++++++++++++++++---------- 4 files changed, 31 insertions(+), 26 deletions(-) -- 1.7.4.2 |
|
From: David S. <da...@re...> - 2011-04-08 11:39:57
|
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.
Signed-off-by: David Sommerseth <da...@re...>
---
win/config.h.in | 3 ---
win32.c | 36 ++++++++++++++++++++++++++----------
2 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/win/config.h.in b/win/config.h.in
index abf71d2..fb349d0 100644
--- a/win/config.h.in
+++ b/win/config.h.in
@@ -286,9 +286,6 @@ typedef unsigned long in_addr_t;
/* Route command */
#define ROUTE_PATH "route"
-/* Default temporary directory, if not found via getenv() */
-#define DEFAULT_TMPDIR "C:\\WINDOWS\\Temp" /* FIXME: Should be configurable */
-
#ifdef _MSC_VER
/* MSVC++ hacks */
#pragma warning(disable:4244) // conversion from 'foo' to 'bar', possible loss of data
diff --git a/win32.c b/win32.c
index b65f987..29653da 100644
--- a/win32.c
+++ b/win32.c
@@ -1097,19 +1097,35 @@ env_set_add_win32 (struct env_set *es)
const char *
win_get_tempdir()
{
- static char *tmpdir = NULL;
+ static char tmpdir[258];
+ char *envptr = NULL;
- /* Try to use %TEMP% or %TMP% */
- tmpdir = getenv("TEMP");
- if( !tmpdir ) {
- tmpdir = getenv("TMP");
+ CLEAR (tmpdir);
+
+ /* 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;
+
+ envptr = getenv("TMP");
+ if( envptr ) {
+ return envptr;
}
+
+ 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;
}
#endif
--
1.7.4.2
|
|
From: Gert D. <ge...@gr...> - 2011-04-08 15:45:59
|
Hi,
On Fri, Apr 08, 2011 at 01:40:05PM +0200, David Sommerseth wrote:
> 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.
ACK in principle, but
> + return NULL; /* Just in case M_FATAL doesn't exit() */
NACK on this.
We don't do this anywhere else where msg(M_FATAL) is called either - because
we know it won't return.
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany ge...@gr...
fax: +49-89-35655025 ge...@ne...
|
|
From: Alon Bar-L. <alo...@gm...> - 2011-04-08 15:48:08
|
On Fri, Apr 8, 2011 at 2:40 PM, David Sommerseth <da...@re...> wrote:
> +
> + 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;
> }
Why something wrong with Windows?
This is incorrect.
Also, there should be no warning, as it perfectly standard location.
And I don't see ExpandEnvironmentVariables() usage, I expected to have
something to expand %SystemRoot%.
|
|
From: Gisle V. <gv...@br...> - 2011-04-08 16:03:17
|
"David Sommerseth" <da...@re...> wrote:
> - /* Try to use %TEMP% or %TMP% */
> - tmpdir = getenv("TEMP");
> - if( !tmpdir ) {
> - tmpdir = getenv("TMP");
> + CLEAR (tmpdir);
> +
> + /* 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.
--gv
|
|
From: Alon Bar-L. <alo...@gm...> - 2011-04-08 16:32:54
|
On Fri, Apr 8, 2011 at 7:03 PM, Gisle Vanem <gv...@br...> wrote: > 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 : Good catch. Alon. |
|
From: David S. <ope...@to...> - 2011-04-08 22:56:24
|
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/04/11 18:03, Gisle Vanem wrote:
> "David Sommerseth" <da...@re...> wrote:
>
>> - /* Try to use %TEMP% or %TMP% */
>> - tmpdir = getenv("TEMP");
>> - if( !tmpdir ) {
>> - tmpdir = getenv("TMP");
>> + CLEAR (tmpdir);
>> +
>> + /* 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.
It's most probably very obvious that I'm not a Windows developer by now :)
I completely agree, the docs looks reasonable and we're also not supporting
Windows 2000 or older. No need to reimplement core system features.
To avoid that I'm sending out yet another patch which will need to be fixed
again, can you please submit a proper patch for it? I've never done
anything big on Windows with C before, so when it comes to specific Windows
API features, I'm completely blank. For me the type LPTSTR tells me
nothing, I just presume it is some kind of Windows' equivalent of char *,
and that DWORD is some kind of 32 bit integer. But I might be mistaken again.
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk2fkq0ACgkQDC186MBRfrqMaACeOLOGDHo8hVkz89Np+NOxkQLX
9W0AoIRYdC0SwpAiYqN2Zy4HS0v+dhBd
=bWuY
-----END PGP SIGNATURE-----
|
|
From: David S. <da...@re...> - 2011-04-08 11:39:57
|
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. With this path the temporary directory can be defined by either setting $TMPDIR or using the --tmp-dir option. Otherwise, /tmp will be the default [1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03 Signed-off-by: David Sommerseth <da...@re...> --- configure.ac | 11 ----------- options.c | 7 +++++-- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/configure.ac b/configure.ac index a9f022e..e0847bc 100644 --- a/configure.ac +++ b/configure.ac @@ -279,11 +279,6 @@ AC_ARG_WITH(netstat-path, ) AC_DEFINE_UNQUOTED(NETSTAT_PATH, "$NETSTAT", [Path to netstat tool]) -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"] -) - AC_ARG_WITH(mem-check, [ --with-mem-check=TYPE Build with debug memory checking, TYPE = dmalloc or valgrind], [MEMCHECK="$withval"] @@ -571,12 +566,6 @@ LDFLAGS="$LDFLAGS -Wl,--fatal-warnings" AC_CHECK_FUNC(epoll_create, AC_DEFINE(HAVE_EPOLL_CREATE, 1, [epoll_create function is defined])) LDFLAGS="$OLDLDFLAGS" -dnl set the default temporary directory -if test -z "$TMPDIRPATH"; then - TMPDIRPATH="/tmp" -fi -AC_DEFINE_UNQUOTED(DEFAULT_TMPDIR, "$TMPDIRPATH", [Default --tmp-dir]) - dnl dnl check for valgrind tool dnl diff --git a/options.c b/options.c index 0b91657..7303cb4 100644 --- a/options.c +++ b/options.c @@ -777,8 +777,11 @@ init_options (struct options *o, const bool init_gc) /* On Windows, find temp dir via enviroment variables */ o->tmp_dir = win_get_tempdir(); #else - /* Non-windows platforms have this value defined via ./configure */ - o->tmp_dir = DEFAULT_TMPDIR; + /* 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 */ } -- 1.7.4.2 |
|
From: Gert D. <ge...@gr...> - 2011-04-08 15:41:49
|
Hi,
On Fri, Apr 08, 2011 at 01:40:04PM +0200, David Sommerseth wrote:
> 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.
ACK.
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany ge...@gr...
fax: +49-89-35655025 ge...@ne...
|
|
From: Carsten K. <C.K...@gm...> - 2011-04-08 16:00:34
|
Hello David, > 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 think that's not the right location. Use http://msdn.microsoft.com/en-us/library/system.environment.getfolderpath.aspx with this constant CSIDL_LOCAL_APPDATA to locate system/language independant: "C:\Documents and Settings\username\Local Settings\Application Data" http://msdn.microsoft.com/en-us/library/bb762494.aspx and than create OpenVPN\temp at this location. Windows has no special temp location that is "allowed" from MS. greetings Carsten |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X