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

Fix Some Issues, Improve Code quality #3051

Merged
merged 8 commits into from Apr 26, 2017

Conversation

Projects
None yet
3 participants
@Kissaki
Member

Kissaki commented Apr 25, 2017

Fixing some stuff I found with PVS-Studio.

I also noticed: In src/mumble we have EnvUtils to handle getting environment variables. This should be moved to src, so the code in src/murmur can also make use of it (there’s another place I didn't touch as well).

CryptState also still has two fields which I don’t know how to correctly initialize (openssh types), so for now I didn’t (I believe these missing init. were pointed to by cppcheck as well).

Kissaki added some commits Apr 25, 2017

Use readable hex version constant for checks
The octal representation 0201003 of version 1.2.3 is hardly readable.
Hexadecimal representations are used elsewhere because they are easily
readable (0x010203).

The version is a 32 bit number with 2 bytes major, 1 byte minor, 1 byte
patch - see Mumble.proto.

@Kissaki Kissaki requested review from mkrautz and davidebeatrici Apr 25, 2017

Show outdated Hide outdated src/mumble/EnvUtils.cpp
_wgetenv_s(&requiredSize, 0, 0, wname);
errno_t res = _wgetenv_s(&requiredSize, 0, 0, wname);
if (res != 0) {
QString();

This comment has been minimized.

@davidebeatrici

davidebeatrici Apr 25, 2017

Member

Shouldn't this be return QString()?

@davidebeatrici

davidebeatrici Apr 25, 2017

Member

Shouldn't this be return QString()?

This comment has been minimized.

@Kissaki

Kissaki Apr 25, 2017

Member

Yes, fixed. I also dropped the res variable.

@Kissaki

Kissaki Apr 25, 2017

Member

Yes, fixed. I also dropped the res variable.

Show outdated Hide outdated src/mumble/EnvUtils.cpp
_wgetenv_s(&requiredSize, reinterpret_cast<wchar_t *>(buf.data()), requiredSize, wname);
errno_t res = _wgetenv_s(&requiredSize, reinterpret_cast<wchar_t *>(buf.data()), requiredSize, wname);
if (res != 0) {
QString();

This comment has been minimized.

@davidebeatrici

davidebeatrici Apr 25, 2017

Member

As above.

@davidebeatrici

This comment has been minimized.

@Kissaki

Kissaki Apr 25, 2017

Member

Yes, fixed. I also dropped the res variable.

@Kissaki

Kissaki Apr 25, 2017

Member

Yes, fixed. I also dropped the res variable.

Kissaki added some commits Apr 25, 2017

Check for error on winapi calls
Although we could handle some errors in place (value got longer),
or they otherwise indicate a usage/coding problem,
return an empty string as if no environment variable were found.
Check for null before pointer use
If pDevice can not be opened (is NULL), we jump to cleanup. That would
result in a null pointer dereferenciation.
Reduce variable scope
Declare variable pDevice after block where it is not used,
but before block where we define/look for it.
Add comment for workaround from/as in mumble/main.cpp
(Let’s hope that is the only reason that code is there!?)
Show outdated Hide outdated src/murmur/main.cpp
_wgetenv_s(&reqSize, NULL, 0, L"PATH");
if (reqSize > 0) {
if (_wgetenv_s(&reqSize, NULL, 0, L"PATH") != 0) {
qWarn() << "Failed to get PATH. Not adding application directory to PATH. DBus bindings may not work.";

This comment has been minimized.

@Kissaki

Kissaki Apr 25, 2017

Member

Missing include

@Kissaki

Kissaki Apr 25, 2017

Member

Missing include

This comment has been minimized.

@Kissaki

Kissaki Apr 25, 2017

Member

Actually not include, it's qWarning(), not qWarn() :)

@Kissaki

Kissaki Apr 25, 2017

Member

Actually not include, it's qWarning(), not qWarn() :)

@@ -294,7 +294,7 @@ bool VoiceRecorder::ensureFileIsOpenedFor(SF_INFO& soundFileInfo, boost::shared_
void VoiceRecorder::run() {
Q_ASSERT(!m_recording);
if (g.sh && g.sh->uiVersion < 0201003)

This comment has been minimized.

@mkrautz

mkrautz Apr 26, 2017

Member

WTF? Why did we do this in the first place?!

@mkrautz

mkrautz Apr 26, 2017

Member

WTF? Why did we do this in the first place?!

@@ -1065,7 +1065,9 @@ void WASAPIOutput::run() {
if (pwfx)
CoTaskMemFree(pwfx);
setVolumes(pDevice, false);
if (pDevice) {
setVolumes(pDevice, false);

This comment has been minimized.

@mkrautz

mkrautz Apr 26, 2017

Member

setVolumes is called in other contexts where you don't do the null check.
Maybe it should be moved into the function itself?

@mkrautz

mkrautz Apr 26, 2017

Member

setVolumes is called in other contexts where you don't do the null check.
Maybe it should be moved into the function itself?

This comment has been minimized.

@Kissaki

Kissaki Apr 26, 2017

Member

It’s only being called in WASAPIOutput::run() (3 times), after the initial pDevice null check which jumps to cleanup.

@Kissaki

Kissaki Apr 26, 2017

Member

It’s only being called in WASAPIOutput::run() (3 times), after the initial pDevice null check which jumps to cleanup.

@Kissaki Kissaki merged commit 5489564 into mumble-voip:master Apr 26, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Kissaki Kissaki deleted the Kissaki:code-quality branch Apr 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment