| Message ID | 1511026858-23281-2-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,1/2] Ensure strings read from registry are null-terminated | expand |
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
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
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
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
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
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
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
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
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 --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;