Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 50 additions & 2 deletions src/mumble/MumbleApplication.cpp
Expand Up @@ -11,6 +11,54 @@
#include "GlobalShortcut.h"
#include "Global.h"

// getenvQString is a wrapper around _wgetenv_s (on Windows)
// and getenv (on everything else).
//
// On Windows, it expects a Unicode environment -- so variables
// are expected to be UTF16.
// On everthing else, it expects the environment variables to be
// UTF-8 encoded.
static QString getenvQString(QString name) {
#ifdef Q_OS_WIN
QByteArray buf;
size_t requiredSize = 0;

static_assert(sizeof(wchar_t) == sizeof(ushort), "expected 2-byte wchar_t");

const wchar_t *wname = reinterpret_cast<const wchar_t *>(name.utf16());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Query the required buffer size (in elements).
_wgetenv_s(&requiredSize, 0, 0, wname);
if (requiredSize == 0) {
return QString();
}

// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Find the length of the buffer without
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// counting NUL elements.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const wchar_t *wbuf = reinterpret_cast<const wchar_t *>(buf.constData());
size_t len = 0;
for (len = 0; len < requiredSize; len++) {
if (wbuf[len] == 0) {
break;
}
}

// Convert the value to QString and return it.
return QString::fromWCharArray(wbuf, static_cast<int>(len));
#else
QByteArray nameU8 = name.toUtf8();
char *val = ::getenv(nameU8.constData());
if (val == NULL) {
return QString();
}
return QString::fromUtf8(val);
#endif
}

MumbleApplication *MumbleApplication::instance() {
return static_cast<MumbleApplication *>(QCoreApplication::instance());
}
Expand All @@ -25,9 +73,9 @@ MumbleApplication::MumbleApplication(int &pargc, char **pargv)
}

QString MumbleApplication::applicationVersionRootPath() {
QByteArray versionRoot = qgetenv("MUMBLE_VERSION_ROOT");
QString versionRoot = getenvQString(QLatin1String("MUMBLE_VERSION_ROOT"));
if (versionRoot.count() > 0) {
return QString::fromUtf8(versionRoot.constData());
return versionRoot;
}
return this->applicationDirPath();
}
Expand Down