diff mbox series

[Openvpn-devel,2/2] Make most registry values optional

Message ID 1511026858-23281-2-git-send-email-selva.nair@gmail.com
State Accepted
Headers
Series [Openvpn-devel,1/2] Ensure strings read from registry are null-terminated |

Commit Message

Selva Nair Nov. 18, 2017, 6:40 a.m. UTC 
  From: Selva Nair <selva.nair@gmail.com>

Not all installations need registry values such as log_dir and
config_dir especially if automatic service is not in use.
This patch provides reasonable defaults for registry values.

- Read the default value of HKLM\Software\PACKAGE_NAME to get the
  install path and construct defaults for exe_path, config_dir,
  log_dir from it. Use "ovpn", "0", NORMAL_PRIORITY as the defaults
  for config file extension, log-append flag and process priority.

The only remaining required registry entry is the root key (usually
HKLM\Software\OpenVPN) whose default value should be set to the
installation path.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpnserv/common.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

Comments

Gert Doering Feb. 20, 2018, 1:23 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

"Because it makes sense".  Stared at the code, compile-tested on 16.04.

Same issue as in 1/2 with the "reg_path" change...  but again, only
conflicts in context, not actual code, and context that is not affected 
by the code changes.

Your patch has been applied to the master and release/2.4 branch.

commit db04bca6729e9fe1ea60f0b3bd0329244a6ed611 (master)
commit c07bdace820753ab29def5edc3a6cadbf233cd03 (release/2.4)
Author: Selva Nair
Date:   Sat Nov 18 12:40:58 2017 -0500

     Make most registry values optional

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <1511026858-23281-2-git-send-email-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15892.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Feb. 20, 2018, 5:14 a.m. UTC | #2 
Hi,

well, today is the day that I get to break things, it seems...

On Sat, Nov 18, 2017 at 12:40:58PM -0500, selva.nair@gmail.com wrote:
> +    /* The default value of REG_KEY is the install path */
> +    if (GetRegString(key, NULL, install_path, sizeof(install_path), NULL) != ERROR_SUCCESS)
> +    {
> +        goto out;
> +    }

This seems to break on Win7/64 for me.

Trying to start the latest iservice from Samuli's snapshot builds (master, 
but I assume release/2.4 is also affected) returns a non-useful error 
message.

Looking into the event viewer, it tells me:

	openvpnserv error: Falscher Parameter. (0x57)
	Error querying registry value: HKLM\SOFTWARE\OpenVPN\(null)

("Falscher Parameter" = "invalid argument")


... so maybe this is not the right way to query the default path?

gert
Selva Nair Feb. 20, 2018, 5:19 a.m. UTC | #3 
Hi,

On Tue, Feb 20, 2018 at 11:14 AM, Gert Doering <gert@greenie.muc.de> wrote:
> Hi,
>
> well, today is the day that I get to break things, it seems...
>
> On Sat, Nov 18, 2017 at 12:40:58PM -0500, selva.nair@gmail.com wrote:
>> +    /* The default value of REG_KEY is the install path */
>> +    if (GetRegString(key, NULL, install_path, sizeof(install_path), NULL) != ERROR_SUCCESS)
>> +    {
>> +        goto out;
>> +    }
>
> This seems to break on Win7/64 for me.
>
> Trying to start the latest iservice from Samuli's snapshot builds (master,
> but I assume release/2.4 is also affected) returns a non-useful error
> message.
>
> Looking into the event viewer, it tells me:
>
>         openvpnserv error: Falscher Parameter. (0x57)
>         Error querying registry value: HKLM\SOFTWARE\OpenVPN\(null)
>
> ("Falscher Parameter" = "invalid argument")
>
>
> ... so maybe this is not the right way to query the default path?


Hmm.. I thought I had tested the patch.. Looking at it right now -- hold on :)

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Feb. 20, 2018, 8:59 a.m. UTC | #4 
Hi,

On Tue, Feb 20, 2018 at 11:19:23AM -0500, Selva Nair wrote:
> Hmm.. I thought I had tested the patch.. Looking at it right now -- hold on :)

Missed you on IRC...

18:21 < selvanair> cron2: iservives in 2.4 local build and snapshot build works 
                   on Windows 10. Have to go now, but will test Win7 later 
                   today when I can get hold of a one.
18:22 < selvanair> s/iservives/iservice

... seems this is different on Win7...?

To be sure it's not something messed up on my system, I have checked
the HKLM\Software\OpenVPN\(null) path that it's trying to access - and
all is there

(standard)  REG_SZ  c:\Program Files\OpenVPN
config_dir  REG_SZ  c:\Program Files\OpenVPN\config
...


and according to the documentation in

https://msdn.microsoft.com/en-us/library/windows/desktop/ms724868(v=vs.85).aspx

passing "lpValue = NULL" really should work...  the error code 0x57
OTOH is documented as

	ERROR_INVALID_PARAMETER
	87 (0x57)
	The parameter is incorrect.

... which isn't exactly helpful to see *which* argument it does not like 
- everything *should* be according to specification...

(It does happen for my own binary and for the installers Samuli builds,
so it's not "my build environment" - theoretically it could be part 1/2
of that patch set, which I haven't run yet)

gert
Selva Nair Feb. 20, 2018, 12:33 p.m. UTC | #5 
Hi,

On Tue, Feb 20, 2018 at 2:59 PM, Gert Doering <gert@greenie.muc.de> wrote:
> Hi,
>
> On Tue, Feb 20, 2018 at 11:19:23AM -0500, Selva Nair wrote:
>> Hmm.. I thought I had tested the patch.. Looking at it right now -- hold on :)
>
> Missed you on IRC...
>
> 18:21 < selvanair> cron2: iservives in 2.4 local build and snapshot build works
>                    on Windows 10. Have to go now, but will test Win7 later
>                    today when I can get hold of a one.
> 18:22 < selvanair> s/iservives/iservice
>
> ... seems this is different on Win7...?
>
> To be sure it's not something messed up on my system, I have checked
> the HKLM\Software\OpenVPN\(null) path that it's trying to access - and
> all is there
>
> (standard)  REG_SZ  c:\Program Files\OpenVPN
> config_dir  REG_SZ  c:\Program Files\OpenVPN\config
> ...
>
>
> and according to the documentation in
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms724868(v=vs.85).aspx
>
> passing "lpValue = NULL" really should work...  the error code 0x57
> OTOH is documented as
>
>         ERROR_INVALID_PARAMETER
>         87 (0x57)
>         The parameter is incorrect.
>
> ... which isn't exactly helpful to see *which* argument it does not like
> - everything *should* be according to specification...
>
> (It does happen for my own binary and for the installers Samuli builds,
> so it's not "my build environment" - theoretically it could be part 1/2
> of that patch set, which I haven't run yet)

Yeah, I could reproduce it on Windows 7. And, at last,  after several
painful Windows builds, it turns out:

RegGetValue with RRF_RT_REG_EXPAND_SZ|RRF_RT_REG_SZ is broken on
Windows 7 (and most likely Vista too).

But once you know what to look for, just google:

https://stackoverflow.com/questions/47096940/reggetvalue-for-value-that-may-be-either-reg-sz-or-reg-expand-sz

Will send a patch later.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Feb. 20, 2018, 8:42 p.m. UTC | #6 
Hi,

On Tue, Feb 20, 2018 at 06:33:35PM -0500, Selva Nair wrote:
> > (It does happen for my own binary and for the installers Samuli builds,
> > so it's not "my build environment" - theoretically it could be part 1/2
> > of that patch set, which I haven't run yet)
> 
> Yeah, I could reproduce it on Windows 7. And, at last,  after several
> painful Windows builds, it turns out:
> 
> RegGetValue with RRF_RT_REG_EXPAND_SZ|RRF_RT_REG_SZ is broken on
> Windows 7 (and most likely Vista too).

Thanks for investigating and finding the root cause.  "This is not what
the documentation says...!" -> I need to actually *run* these things, not
just test compile.

> But once you know what to look for, just google:
> 
> https://stackoverflow.com/questions/47096940/reggetvalue-for-value-that-may-be-either-reg-sz-or-reg-expand-sz
> 
> Will send a patch later.

Thanks!

PS: when you say "several painful Windows builds" - I found that things
are actually working quite nicely using "build-snapshot --use-depcache",
pointing this to a private git repo which has the code I want to test.
Still a bit annoying because the machinery insists on "fetch all, clean
all, rebuild all" instead of "there's the source directory, just run 
make!"... shall we try to spend a bit time on this, to make interactively
working on source changes nicer?

gert
Selva Nair Feb. 21, 2018, 2:20 a.m. UTC | #7 
Hi,

On Wed, Feb 21, 2018 at 2:42 AM, Gert Doering <gert@greenie.muc.de> wrote:
> Hi,
>
> On Tue, Feb 20, 2018 at 06:33:35PM -0500, Selva Nair wrote:
>> > (It does happen for my own binary and for the installers Samuli builds,
>> > so it's not "my build environment" - theoretically it could be part 1/2
>> > of that patch set, which I haven't run yet)
>>
>> Yeah, I could reproduce it on Windows 7. And, at last,  after several
>> painful Windows builds, it turns out:
>>
>> RegGetValue with RRF_RT_REG_EXPAND_SZ|RRF_RT_REG_SZ is broken on
>> Windows 7 (and most likely Vista too).
>
> Thanks for investigating and finding the root cause.  "This is not what
> the documentation says...!" -> I need to actually *run* these things, not
> just test compile.

When I say painful builds, I mean edit build[*], copy to Windows, run
in a few configurations, look through the event viewer, rinse rather
repeat. And finally when it seems to start working connect to a my
work VPN,  remove the registry keys and try to break it etc..

FYI, I don't just "test compile", and leave it at that.

Selva

[*] Yes, that fetch-all and rebuild from scratch part of openvpn-build
is very time consuming. Making it work the usual "remake only what
changed" way would save a lot of time and frustration.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair Feb. 21, 2018, 2:40 a.m. UTC | #8 
Hi,

On Wed, Feb 21, 2018 at 8:20 AM, Selva Nair <selva.nair@gmail.com> wrote:
> Hi,
>
> On Wed, Feb 21, 2018 at 2:42 AM, Gert Doering <gert@greenie.muc.de> wrote:
>> Hi,
>>
>> On Tue, Feb 20, 2018 at 06:33:35PM -0500, Selva Nair wrote:
>>> > (It does happen for my own binary and for the installers Samuli builds,
>>> > so it's not "my build environment" - theoretically it could be part 1/2
>>> > of that patch set, which I haven't run yet)
>>>
>>> Yeah, I could reproduce it on Windows 7. And, at last,  after several
>>> painful Windows builds, it turns out:
>>>
>>> RegGetValue with RRF_RT_REG_EXPAND_SZ|RRF_RT_REG_SZ is broken on
>>> Windows 7 (and most likely Vista too).
>>
>> Thanks for investigating and finding the root cause.  "This is not what
>> the documentation says...!" -> I need to actually *run* these things, not
>> just test compile.
>
> When I say painful builds, I mean edit build[*], copy to Windows, run
> in a few configurations, look through the event viewer, rinse rather
> repeat. And finally when it seems to start working connect to a my
> work VPN,  remove the registry keys and try to break it etc..
>
> FYI, I don't just "test compile", and leave it at that.

On re-reading, it sounds different from what I wanted to say. I was
just cursing stupid Windows that's no fun to debug on... In this
particular case it was my fault that I never tested the patch on
Windows 7 in the first place. I used to run a Win7 VM, but now all my
desktop memory is eaten by those 50 chrome tabs..

Anyway, sorry to all (and to myself) for causing a "must-fix" bug
close to a release.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Feb. 21, 2018, 2:45 a.m. UTC | #9 
Hi,

On Wed, Feb 21, 2018 at 08:20:34AM -0500, Selva Nair wrote:
> > Thanks for investigating and finding the root cause.  "This is not what
> > the documentation says...!" -> I need to actually *run* these things, not
> > just test compile.
> 
> When I say painful builds, I mean edit build[*], copy to Windows, run
> in a few configurations, look through the event viewer, rinse rather
> repeat. And finally when it seems to start working connect to a my
> work VPN,  remove the registry keys and try to break it etc..

Indeed, that takes quite a bit of time.

> FYI, I don't just "test compile", and leave it at that.

I did not want to imply that you did - sorry if it came across that way.  

*I* only test compiled, and was then happy merging your patch ("I'm sure 
Selva tested this, it is according to documentation and the code looks 
good, and compiles fine") - but as I learned, there are more caveats on
Windows.  So, when we do more basic changes (new API, etc), I need to
do actual test installations on my windows zoo, like, win7/32, win7/64,
win10/64...  I usually do this already today if I suspect breakage, but
in this case, it came as a surprise.

gert
diff mbox series

Patch

diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index bc99584..759e6a0 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -87,6 +87,8 @@  GetOpenvpnSettings(settings_t *s)
     TCHAR append[2];
     DWORD error;
     HKEY key;
+    TCHAR install_path[MAX_PATH];
+    TCHAR default_value[MAX_PATH];
 
     LONG status = RegOpenKeyEx(HKEY_LOCAL_MACHINE, REG_KEY, 0, KEY_READ, &key);
     if (status != ERROR_SUCCESS)
@@ -95,37 +97,50 @@  GetOpenvpnSettings(settings_t *s)
         return MsgToEventLog(M_SYSERR, TEXT("Could not open Registry key HKLM\\%s not found"), REG_KEY);
     }
 
-    error = GetRegString(key, TEXT("exe_path"), s->exe_path, sizeof(s->exe_path), NULL);
+    /* The default value of REG_KEY is the install path */
+    if (GetRegString(key, NULL, install_path, sizeof(install_path), NULL) != ERROR_SUCCESS)
+    {
+        goto out;
+    }
+
+    openvpn_sntprintf(default_value, _countof(default_value), TEXT("%s\\bin\\openvpn.exe"),
+                      install_path);
+    error = GetRegString(key, TEXT("exe_path"), s->exe_path, sizeof(s->exe_path), default_value);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("config_dir"), s->config_dir, sizeof(s->config_dir), NULL);
+    openvpn_sntprintf(default_value, _countof(default_value), TEXT("%s\\config"), install_path);
+    error = GetRegString(key, TEXT("config_dir"), s->config_dir, sizeof(s->config_dir),
+                         default_value);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("config_ext"), s->ext_string, sizeof(s->ext_string), NULL);
+    error = GetRegString(key, TEXT("config_ext"), s->ext_string, sizeof(s->ext_string),
+                         TEXT(".ovpn"));
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir), NULL);
+    openvpn_sntprintf(default_value, _countof(default_value), TEXT("%s\\log"), install_path);
+    error = GetRegString(key, TEXT("log_dir"), s->log_dir, sizeof(s->log_dir), default_value);
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("priority"), priority, sizeof(priority), NULL);
+    error = GetRegString(key, TEXT("priority"), priority, sizeof(priority),
+                         TEXT("NORMAL_PRIORITY_CLASS"));
     if (error != ERROR_SUCCESS)
     {
         goto out;
     }
 
-    error = GetRegString(key, TEXT("log_append"), append, sizeof(append), NULL);
+    error = GetRegString(key, TEXT("log_append"), append, sizeof(append), TEXT("0"));
     if (error != ERROR_SUCCESS)
     {
         goto out;