MumbleApplication: introduce getenvQString and use it in applicationVersionRoot. #2808

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@mkrautz
Member
mkrautz commented Feb 3, 2017

When built against the MSVC2015 CRT, we can't mix-and-match non-wide and
wide environment variables anymore.

Prior to this commit, MumbleApplication::applicationVersionRoot() used
Qt's qgetenv().

However, because of our change to MSVC2015, we can't use Qt's function
anymore.

Instead, we introduce getenvQString (a static function, local to
MumbleApplication -- for now). This function uses _wgetenv_s on
Windows to retrieve environment variables (expecting that they're all
UTF-16). On non-Windows systems, it uses getenv (and expects keys and
values to be UTF-8).

Fixes mumble-voip/mumble#2806

@davidebeatrici davidebeatrici added the build label Feb 3, 2017
@mkrautz mkrautz MumbleApplication: introduce getenvQString and use it in applicationV…
…ersionRoot.

When built against the MSVC2015 CRT, we can't mix-and-match non-wide and
wide environment variables anymore.

Prior to this commit, MumbleApplication::applicationVersionRoot() used
Qt's qgetenv().

However, because of our change to MSVC2015, we can't use Qt's function
anymore.

Instead, we introduce getenvQString (a static function, local to
MumbleApplication -- for now). This function uses _wgetenv_s on
Windows to retrieve environment variables (expecting that they're all
UTF-16). On non-Windows systems, it uses getenv (and expects keys and
values to be UTF-8).

Fixes mumble-voip/mumble#2806
79b25ca
@mkrautz mkrautz requested review from Kissaki and hacst Feb 3, 2017
src/mumble/MumbleApplication.cpp
+
+ // Resize buf to fit the value and put it there.
+ buf.resize(static_cast<int>(requiredSize * sizeof(wchar_t)));
+ _wgetenv_s(&requiredSize, reinterpret_cast<wchar_t *>(buf.data()), requiredSize * sizeof(wchar_t), wname);
@Kissaki
Kissaki Feb 4, 2017 Member

The size paramter is numberOfElements "Size of buffer.". Shouldn't that be requiredSize as _wgetenv_s will read wchar_ts, of which we get requiredSize of them?

@hacst
hacst Feb 4, 2017 Member

Totally missed that. Yeah. That should be fixed (even though it can't really break anything outside of a race condition).

src/mumble/MumbleApplication.cpp
+ _wgetenv_s(&requiredSize, reinterpret_cast<wchar_t *>(buf.data()), requiredSize * sizeof(wchar_t), wname);
+
+ // Find the length of the buffer without
+ // counting NUL elements.
@Kissaki
Kissaki Feb 4, 2017 Member

Where would we get NUL elements from after we made buf the size it needs to be for all its content?

@hacst
hacst approved these changes Feb 4, 2017 View changes

Can't compile test it but looks sane.

src/mumble/MumbleApplication.cpp
+ buf.resize(static_cast<int>(requiredSize * sizeof(wchar_t)));
+ _wgetenv_s(&requiredSize, reinterpret_cast<wchar_t *>(buf.data()), requiredSize * sizeof(wchar_t), wname);
+
+ // Find the length of the buffer without
@hacst
hacst Feb 4, 2017 Member

Wouldn't have QString::fromWCharArray(wbuf); done the trick? This is an old C-API (albeit widestring) so I can't imagine it not being zero terminated. If it isn't there's still wcsnlen_s that does the same as the code below.

Not that the code is wrong. So no use blocking the merge.

@@ -25,9 +73,9 @@ MumbleApplication::MumbleApplication(int &pargc, char **pargv)
}
QString MumbleApplication::applicationVersionRootPath() {
- QByteArray versionRoot = qgetenv("MUMBLE_VERSION_ROOT");
@hacst
hacst Feb 4, 2017 Member

So whose bug is this? This function not working like before surely breaks lots of other stuff out there.

@hacst hacst Integrate review comments on getenvQString
* Use correct size in second _wgetenv_s call
* Rely on zero termination being present in buffer to get rid of some code
a3673e5
@hacst
Member
hacst commented Feb 4, 2017

Now that is interesting. For some reason I can actually commit to this PR directly from github. No idea why it lets me "push" to mkrautz's branch at all ;)

@Kissaki PTAL . If you have a way to actually test compile and run this that would be great. I could only test the basic behavior of the _wgetenv_s calls.

@mkrautz How do we handle that going forward? Pushing to other people's branches seems a bit strange. At least without us having talked about whether we want to do so.

@Kissaki
Member
Kissaki commented Feb 4, 2017 edited

@hacst When you create a PR you have a checkbox to allow or not allow repo maintainers to push to your fork branch.

If checked, users with write access to mumble-voip/mumble can add new commits to your […] branch. You can always change this setting later.

@mkrautz
Member
mkrautz commented Feb 4, 2017

Allow edits is on.

The docs don't say that it zero terminates, that's why I left it.

Still AFK. Please handle this if possible.

@hacst
Member
hacst commented Feb 4, 2017

Will try. The problem is that neither of us can actually build a usable windows snapshot and installer to test with right now. And if we somehow make the client crash then no more auto-updates...

@hacst
Member
hacst commented Feb 4, 2017

Did not figure out how to merge this pull with rebase so I did it manually.

Merged as:
1773dc7 Integrate review comments on getenvQString
11f9244 MumbleApplication: introduce getenvQString and use it in applicationVersionRoot.

@hacst hacst closed this Feb 4, 2017
@Kissaki
Member
Kissaki commented Feb 5, 2017

Why did you want to rebase instead of merging anyway?

@hacst
Member
hacst commented Feb 5, 2017

Isn't that still our preferred way to merge?

@mkrautz
Member
mkrautz commented Feb 5, 2017
18:38:22 <mkrautz> hacst: yes, I now use merge commits every time I merge... I like that (in theory) it     groups the commits, and leaves a note which PR they were from
18:39:24 <mkrautz> and I always merge via the GitHub UI because I'm too dumb to merge by command  line
18:39:39 <mkrautz> (sometimes my master branch is not up-to-date, or has junk in it)
18:39:42 <mkrautz> (github merge is always pristine)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment