static analysis fixes #553

ebudai · 2019-05-08T02:50:16Z

No description provided.

msftclas · 2019-05-08T02:50:28Z

All CLA requirements met.

src/host/srvinit.cpp

src/host/utf8ToWideCharParser.cpp

src/interactivity/win32/UiaTextRange.cpp

src/renderer/base/thread.cpp

src/server/ApiMessage.cpp

src/interactivity/win32/window.cpp

src/renderer/vt/tracing.cpp

src/server/ApiMessage.cpp

src/host/screenInfo.cpp

zadjii-msft

I want to make sure both @adiviness and @miniksa get their eyes on this, so I'll let them be the two signoffs. Otherwise looks good to me.

@adiviness

I want to make sure both @adiviness and @miniksa get their eyes on this, so I'll let them be the two signoffs. Otherwise looks good to me.

src/host/ft_uia/KeyPressTests.cs

src/host/utf8ToWideCharParser.cpp

src/interactivity/win32/UiaTextRange.cpp

src/interactivity/win32/UiaTextRange.hpp

miniksa

I'm going to look a bit further at the server IO one when I'm actually at my desk at work to make sure my concern is warranted before signing off.

miniksa · 2019-05-16T04:10:42Z

src/host/srvinit.cpp

@@ -653,22 +653,22 @@ DWORD ConsoleIoThread()
        if (ReplyMsg != nullptr)
        {
            LOG_IF_FAILED(ReplyMsg->ReleaseMessageBuffers());
-        }


I'm not so sure about this one. I think there are API calls that don't return a ReplyMsg. If any of those are called, then we would be stuck.

In that case, with the old code, ReplyMsg->Complete would be a null dereference.

Should be ok now that I see ReadIo() can take nullptr as 1st parameter.

You're right about the null dereference at &ReplyMsg->Complete. I looked back at the original code this was adapted from, and what should happen is:

ReadIo's signature should be taking the PCONSOLE_API_MSG ReplyMsg as parameter one and only attempting to dig out the completion part if it is not null at that level.

We need to keep the } on line 656 because there's technically a situation where the IO could fail for not pipe-disconnected and try to go around the loop again to reattempt the call with there being no reply message at all.

miniksa · 2019-05-16T04:11:11Z

src/host/utf8ToWideCharParser.cpp

@@ -505,5 +508,5 @@ void Utf8ToWideCharParser::_Reset()
 {
    _currentState = _State::Ready;
    _bytesStored = 0;
-    _convertedWideChars.release();


I think this is also in #836 now

src/interactivity/win32/UiaTextRange.cpp

src/server/ApiMessage.cpp

dlong11 · 2019-05-19T02:49:41Z

src/interactivity/win32/UiaTextRange.hpp

@@ -69,6 +67,8 @@ typedef unsigned int Column;
 // the first char of the 0th row in the text buffer row array.
 typedef unsigned int Endpoint;

+constexpr IdType invalid_id = 0;


@adiviness What is the recommended naming scheme for constants? I have seen all uppercase, pascal, lowercase, etc... Which one is correct?

It depends on the scope of the constant and what sort of constant it is. For macros we typically do all caps (ex. FOO_BAR). for constexprs we typically do pascal case (ex. FooBar).

Relevant ISO C++ guideline:

ES.9: Avoid ALL_CAPS names

TL;DR is that since ALL CAPS names are typically used for macros, it's dangerous to use them for anything but macros since it makes it easy for a macro to silently replace or redefine something that's not a macro.

Thanks @binarycrusader - this is why I was asking. I have seen a lot of constexpr that were upper case. 👎 I noticed an issue was already logged. 👍

miniksa

Waiting for correction of srvinit.cpp IO loop to compensate for null reply message and fix the deref problem via the function call/signature.

… srvinit.cpp by changing signature in DeviceComm.h

miniksa

Excellent, thank you.

DHowett-MSFT

Thanks a bunch for doing this!

static analysis fixes

f0e0587