Address issues picked up by Dev15 code analysis #5272

Penguinwizzard · 2018-06-06T00:00:13Z

This is one of the things that was preventing us from moving our jenkins CI to dev15.

boingoing

Looks good, just a couple of questions.

boingoing · 2018-06-06T02:06:25Z

bin/ch/Debugger.cpp

        {
-            script = new char[fileSize + 1];
+            script = new char[(size_t)fileSize + 1];


Is this required?

Array size declarations are size_t, and the analysis gets unhappy if we have a different type used in the addition here before the implicit conversion. This is notable for types shorter than size_t, as the overflow would occur before the conversion to size_t. I believe that on all currently-supported platforms this is indeed not required.

boingoing · 2018-06-06T02:12:15Z

lib/Runtime/Language/SimdBool32x4OperationX86X64.cpp

@@ -11,7 +11,7 @@ namespace Js
    SIMDValue SIMDBool32x4Operation::OpBool32x4(bool x, bool y, bool z, bool w)
    {
        X86SIMDValue x86Result;
-        x86Result.m128i_value = _mm_set_epi32(w * -1, z * -1, y * -1, x * -1);
+        x86Result.m128i_value = _mm_set_epi32((w?1:0) * -1, (z?1:0) * -1, (y?1:0) * -1, (x?1:0) * -1);


Could this be simpler?

_mm_set_epi32(w?-1:0, z?-1:0, y?-1:0, x?-1:0)

Yes, you're correct, I'll fix that in the next update.

boingoing · 2018-06-06T02:15:13Z

lib/Runtime/Library/SharedArrayBuffer.h

@@ -194,7 +194,7 @@ namespace Js

        Waiters * m_waiters;

-        // Below CS is used for synchronizig access in wait/wake API
+        // Below CS is used for synchronizing access in wait/wake API


Prefast finds typos now?

Drive-by clean-up.

That being said, there are tools that spell-check comments, but I doubt that we'd find much benefit from using them.

I was just taking the opportunity to make a joke. We have typos in comments all over but fixing them like this - as a one-off thing - is probably best.

Ah, sorry. 👍

We actually did use some spell checker tools and manual review of comments and names to clean up before we open-sourced ChakraCore. We didn't catch everything and got some PRs early on fixing other typos, and more typos have creeped in since then. I support drive-by clean up. :)

boingoing · 2018-06-06T02:17:58Z

lib/Runtime/Types/DictionaryTypeHandler.cpp

                    getset = true;
                }
                if (descriptor->GetSetterPropertyIndex() != NoSlots)
                {
                    *setter = instance->GetSlot(descriptor->GetSetterPropertyIndex());
+                    if(!getset) {
+                        // if we didn't set the getter above, we need to set it here


getter is an out param, right? How can we get away without setting it on all paths here? Why not just assign it and setter as well to nullptr right at the top of this function and then we don't care if it was set in above conditional?

Due to the _Success_(return) annotation, the outparam annotations are only required to be valid if the function returns true. I agree that it's a bit poorly designed, but the primary goal here was to align things with existing annotations.

Alright but I object to the silliness of that annotation.

Can we have a getter without a setter, or vice versa? I thought that internally we constructed them in pairs, even if one was a stub "I don't exist, why are you calling me" function.

Since if(!getset) looks like new code, can we use the existing convention (and preferred by many) of putting a space in if (?

Also put the { on a new line, while you're at it.

boingoing · 2018-06-06T02:19:13Z

By the way, should this make it into release/1.10 or hold on until after we cut that branch?

Penguinwizzard · 2018-06-06T02:45:00Z

I'm leaning toward putting it in master after the 1.10 cut. The downside of that is that we may end up with changes to release/1.10 breaking the CI when pumped, but that shouldn't be too common.

MSLaguana · 2018-06-06T02:48:52Z

lib/Common/Core/EtwTraceCore.h

@@ -74,12 +74,18 @@ CompileAssert(false)
 #define MCGEN_PRIVATE_ENABLE_CALLBACK_V2(SourceId, ControlCode, Level, MatchAnyKeyword, MatchAllKeyword, FilterData, CallbackContext) \
       EtwCallback(ControlCode, CallbackContext)

+// Work-around for a bug in the instrumentationevents generator
+#pragma prefast(push)
+#pragma prefast(disable:__WARNING_USING_UNINIT_VAR, "The ETW data generated from the manifest includes a default null function which uses unintialized memory.")


Is this because of how our manifest is structured, or would any project using ETW hit this same warning?

I'm not entirely sure. I wasn't able to see a good way to fix it by changing the manifest, so I believe that it may be across many projects using ETW in a similar manner.

MSLaguana · 2018-06-06T02:59:59Z

lib/Common/DataStructures/BaseDictionary.h

+        void _Releases_lock_(cs.cs) EndResize() {}
+    private:
+        // For prefast analysis, we need to have a somewhat similar shape for both locks
+        Field(contentStruct) cs;


This is a bit of a bummer, having to waste some space for no real reason. I'm not super familiar with the semantics of SAL locking annotations; would it make sense to refer to this instead of the actual lock (or the fake lock here), and elsewhere treat the various lock classes as critical sections / things-that-get-locked?

As for the memory concern, how common is this object? We tend not to worry about memory usage in objects that have very few instances. Additionally, maybe we can add TODO comments to clean up issues like this if possible at a later time, since the scope of this PR is to unblock Dev15 PreFAST analysis.

It's on most of our native Dictionary classes. I was also somewhat concerned that it might change the memory layout in a meaningful way, but there are some static asserts that check for that so it seems to be fine.

As for the number of native dictionaries we have, it's probably more like tens than tens of thousands, so the actual impact is probably small, but I don't have solid numbers to back that up at the moment.

I do agree that my comment here shouldn't block the PR.

MSLaguana · 2018-06-06T03:04:41Z

lib/Common/PlatformAgnostic/DateTimeInternal.h

@@ -61,7 +61,7 @@ namespace DateTime
        uint32 lastTimeZoneUpdateTickCount;

        void UpdateTimeZoneInfo();
-        UtilityPlatformData(): lastTimeZoneUpdateTickCount(0) { }
+        UtilityPlatformData() : lastTimeZoneUpdateTickCount(0) { GetTimeZoneInformation(&timeZoneInfo); }


Instead of directly calling GetTimeZoneInformation here, would it be acceptable to call UpdateTimeZoneInfo to do it instead?

The conditional in UpdateTimeZoneInfo may make it such that the analysis can't be sure that we've initialized timeZoneInfo by the end of the constructor, which is what this code tries to avoid.

MSLaguana · 2018-06-06T03:06:44Z

lib/Runtime/Language/ModuleNamespace.h

@@ -61,7 +61,7 @@ namespace Js
        virtual BOOL DeleteItem(uint32 index, PropertyOperationFlags flags) override { return true; }
        virtual BOOL GetEnumerator(JavascriptStaticEnumerator * enumerator, EnumeratorFlags flags, ScriptContext* requestContext, EnumeratorCache * enumeratorCache = nullptr);
        virtual BOOL SetAccessors(PropertyId propertyId, Var getter, Var setter, PropertyOperationFlags flags = PropertyOperation_None) override { return false; }
-        virtual BOOL GetAccessors(PropertyId propertyId, Var *getter, Var *setter, ScriptContext * requestContext) override { return false; }
+        virtual BOOL GetAccessors(PropertyId propertyId, __out Var *getter, __out Var *setter, ScriptContext * requestContext) override { getter = nullptr; setter = nullptr; return false; }


Should this be *getter = nullptr; *setter = nullptr?

Yes, probably, and with a check on whether the pointers passed in are not nullptr before assigning to them.

Ideally these should have been statically verified as non-null because we said Out and not Out_opt, right?

Also, same pattern is in SpreadArgument.h

*getter = nullptr; *setter = nullptr would indeed be more correct, but the next set of commits avoids the issue entirely by making the function _Success_(return) _Check_return_, and not assigning to either, as the result is always false.

MSLaguana · 2018-06-06T03:10:25Z

lib/Runtime/Library/JavascriptProxy.cpp

@@ -2158,6 +2158,7 @@ namespace Js
                    ::Math::DefaultOverflowPolicy();


Should this use UInt16Math instead?

sigatrev · 2018-06-06T17:52:47Z

bin/ch/RuntimeThreadData.cpp

@@ -25,10 +25,17 @@ RuntimeThreadData::RuntimeThreadData()
    this->hevntInitialScriptCompleted = CreateEvent(NULL, TRUE, FALSE, NULL);
    this->hevntReceivedBroadcast = CreateEvent(NULL, FALSE, FALSE, NULL);
    this->hevntShutdown = CreateEvent(NULL, TRUE, FALSE, NULL);
+    this->hSemaphore = nullptr;
+    this->hThread = nullptr;


is there a reason not to do these in an initializer list, or better yet, in class initialization?

Yeah, changed in next commit.

dilijev · 2018-06-06T18:39:15Z

should this make it into release/1.10 or hold on until after we cut that branch?

I'd prefer before the cut (this change included in 1.10) so that 1.10 can have modern CI config enabled.

dilijev · 2018-06-06T18:59:42Z

For reference the payload we'll be merging to update CI so that modern is Dev15 and Legacy is Dev14 is in #4767 -- we'll retarget that to whichever branch this change lands in.

dilijev · 2018-06-07T22:45:02Z

Copyright (c) 2012 Two Blue Cubes Ltd. All rights reserved.

What do we do about making changes to this file into permanent updates -- should we find the source that generated this file and PR back, or just say we need this update and we're not planning to regen this file?

Refers to: bin/External/catch.hpp:6 in 7a9b9ea. [](commit_id = 7a9b9ea, deletion_comment = False)

dilijev · 2018-06-07T22:46:19Z

bin/NativeTests/JsRTApiTest.cpp

@@ -1243,7 +1244,8 @@ namespace JsRTApiTest
            size_t length;
            REQUIRE(JsStringToPointer(nameValue, &name, &length) == JsNoError);

-            CHECK(length == 1);
+            REQUIRE(length == 1);
+#pragma prefast(suppress:__WARNING_MAYBE_UNINIT_VAR, "The require on the previous line should ensure that name[0] is initialized")


__WARNING_MAYBE_UNINIT_VAR [](start = 25, length = 26)

Did not all warnings have these fancy named macros? (There are some you're still suppressing or disabling by number)

Nope, not all of them do. There are a couple cases where I missed using the named macro, though, that I should be fixing up in the next set of commits.

dilijev · 2018-06-07T22:46:46Z

bin/ch/HostConfigFlags.cpp

@@ -96,6 +96,7 @@ void HostConfigFlags::RemoveArg(int& argc, _Inout_updates_to_(argc, argc) LPWSTR
    Assert(index >= 0 && index < argc);
    for (int i = index + 1; i < argc; ++i)
    {
+#pragma prefast(suppress:6385, "Operation is safe but PREfast is difficult to convince")


6385 [](start = 25, length = 4)

suppress warning by name macro instead of number?

Done in next set of commits.

dilijev · 2018-06-07T22:47:00Z

bin/ch/WScriptJsrt.cpp

@@ -35,6 +35,7 @@
 #endif // FreeBSD or unix ?
 #endif // _WIN32 ?

+#pragma prefast(disable:26444, "This warning unfortunately raises false positives when auto is used for declaring the type of an iterator in a loop.")


26444 [](start = 24, length = 5)

suppress by name macro instead of number?

No macro for this one, unfortunately.

MSLaguana · 2018-06-07T22:50:01Z

Regarding the catch.hpp change; looks like their upstream has that fix (or an equivalent fix at least): https://github.com/catchorg/Catch2/blob/master/single_include/catch.hpp#L10054

They even changed for this same reason: catchorg/Catch2@88a6ff0

dilijev · 2018-06-07T22:51:50Z

lib/Common/PlatformAgnostic/DateTimeInternal.h

+            freq(0),
+            fReset(true),
+            fInit(false),
+            fHiResAvailable(true)


was there a warning about initialization order or is this just drive-by cleanup?

It's mostly drive-by; there's a warning in razzle prefast, and it helps for checking that there's no other uninitialized values when they're in the right order. We do not have initialization order warnings enabled in our jenkins runs.

dilijev · 2018-06-07T22:55:24Z

lib/Runtime/Library/JavascriptProxy.cpp

@@ -2178,13 +2179,19 @@ namespace Js
                calleeInfo.Flags = (CallFlags)(calleeInfo.Flags | CallFlags_ExtraArg | CallFlags_NewTarget);
            }

-            for (uint argCount = 0; argCount < args.Info.Count; argCount++)
+            for (ushort argCount = 0; argCount < (ushort)args.Info.Count; argCount++)


args.Info.Count [](start = 57, length = 15)

is it safe to cast args.Info.Count to ushort? If so, can we make args.Info.Count have the correctly-sized type?

Further up around https://github.com/Microsoft/ChakraCore/pull/5272/files#diff-771647fb11e03690664371970f34eeaaR2126 we check that the value fits in a ushort and throw an error if it doesn't. We do allow for larger argument counts in some circumstances though, so args.Info.Count is a uint.

sethbrenith · 2018-06-07T22:58:14Z

lib/Runtime/Library/JavascriptProxy.cpp

+                AnalysisAssert(argCount < (ushort)args.Info.Count);
+                AnalysisAssert(sizeof(Var*) == sizeof(void*));
+                AnalysisAssert(sizeof(Var*) * argCount < sizeof(void*) * newCount);
+#pragma prefast(suppress:__WARNING_WRITE_OVERRUN, "This is a false positive, and all of the above analysis asserts still didn't convince prefast of that.")


have we opened bugs for false positives?

Not yet; I need to determine where we'd open bugs for these, and what would constitute an actionable report.

dilijev

🕐

We had signed/unsigned mixups between the format string and the args.

Note that this includes functional changes to one implementation, as it could in some cases only set the getter or setter and leave the other uninitialized. There's also a couple other function annotation alignment fixes in this commit.

Unfortunately there's a good number of holes, which seem to be due to how the PREfast analysis of the locking works. Someone familiar with the mechanics may be able to do this with fewer suppressions.

A pre-defined function is added to the chakra etw header that does not initialize a buffer before passing it around. It looks ok, but the warning is in a file that we have limited control over. Here I disabled the warning for the whole included file, which seems like the closest we can scope it.

…ter handling

dilijev · 2018-06-28T19:36:59Z

bin/GCStress/WeightedTable.h

@@ -13,14 +13,14 @@ template <class T>
 class WeightedTable
 {
 public:
-    WeightedTable() :
+    WeightedTable() noexcept :


noexcept [](start = 20, length = 8)

Did you mean to add this?

Yeah, it was that or a longer statement to disable the warning for just that line. This took fewer keypresses, and won't complicate any future efforts to go over all the warnings we've disabled.

dilijev · 2018-06-28T22:26:23Z

bin/NativeTests/UnicodeTextTests.cpp

@@ -4,6 +4,10 @@
 //-------------------------------------------------------------------------------------------------------

 #include "stdafx.h"
+#pragma warning(disable:26434)


comments about what these warnings are? (not blocking PR)

Sure, I'll add that in the next change.

dilijev · 2018-06-28T23:51:13Z

lib/Backend/Lower.cpp

@@ -21156,8 +21156,12 @@ Lowerer::GenerateArgOutForStackArgs(IR::Instr* callInstr, IR::Instr* stackArgsIn


 #if defined(_M_IX86)
-     Assert(false);
-#endif
+    // We get a compilation error on x86 due to assiging a negative to a uint


nit: assiging -> assigning

Added in the next change.

dilijev

…analysis Merge pull request #5272 from Penguinwizzard:dev15_prefast_fixes This is one of the things that was preventing us from moving our jenkins CI to dev15.

…by Dev15 code analysis Merge pull request #5272 from Penguinwizzard:dev15_prefast_fixes This is one of the things that was preventing us from moving our jenkins CI to dev15.

Penguinwizzard requested a review from dilijev June 6, 2018 00:03

boingoing approved these changes Jun 6, 2018

View reviewed changes

MSLaguana reviewed Jun 6, 2018

View reviewed changes

sigatrev reviewed Jun 6, 2018

View reviewed changes

dilijev added the Continuous Integration label Jun 7, 2018

dilijev mentioned this pull request Jun 7, 2018

Test updated release/1.10-ci with prefast fixes #5281

Closed

dilijev reviewed Jun 7, 2018

View reviewed changes

sethbrenith reviewed Jun 7, 2018

View reviewed changes

dilijev suggested changes Jun 7, 2018

View reviewed changes

dilijev mentioned this pull request Jun 13, 2018

Test updated release/1.10-ci with prefast fixes #5308

Closed

Penguinwizzard added 6 commits June 20, 2018 17:07

Fix format string inaccuracies.

18f95b7

We had signed/unsigned mixups between the format string and the args.

Align annotations for GetAccessors functions.

95e0f4f

Note that this includes functional changes to one implementation, as it could in some cases only set the getter or setter and leave the other uninitialized. There's also a couple other function annotation alignment fixes in this commit.

Fix type of argument for isalnum to avoid implicit cast warning

ea3af76

Improve locking annotation somewhat

1f57ed5

Unfortunately there's a good number of holes, which seem to be due to how the PREfast analysis of the locking works. Someone familiar with the mechanics may be able to do this with fewer suppressions.

Silence a warning in the jsrtapitests.

321ef76

Penguinwizzard added 14 commits June 20, 2018 17:08

Constructor-initialize some values in RuntimeThreadData

0cd5f64

Suppress a PREfast false positive in config flag handling

6581486

Change types to avoid overflow-related issues in baseline handling

7ad8cc5

Slight change to annotate overflow as unlikely in command line parame…

3878d55

…ter handling

cast before usage in echo

8454ed6

Attempt to satisfy SWB check

e756000

Address review comments

ba0fadf

SWB fix

024bf5c

move to initializer list for runtimethreaddata

b46397c

standardize GetAccessor calls

34b7013

Change suppression number to named macro

83edb62

disable warnings

6285aee

small fixes

62e989e

Get an additional few cases

80438b7

Penguinwizzard force-pushed the dev15_prefast_fixes branch from 7a9b9ea to 2551132 Compare June 26, 2018 23:51

Penguinwizzard changed the base branch from master to release/1.10 June 26, 2018 23:52

Penguinwizzard force-pushed the dev15_prefast_fixes branch from 2551132 to 80438b7 Compare June 26, 2018 23:54

Address remaining issues for enabling dev15 jenkins ci

b3695e0

dilijev reviewed Jun 28, 2018

View reviewed changes

Penguinwizzard added 2 commits June 28, 2018 13:50

Have wabt work in win7

e450902

Disable one more warning in catch.hpp

a157300

dilijev reviewed Jun 28, 2018

View reviewed changes

Address x86-only prefast warning

d3222bd

dilijev reviewed Jun 28, 2018

View reviewed changes

Address CR comments

02a471a

dilijev approved these changes Jun 29, 2018

View reviewed changes

chakrabot merged commit 02a471a into chakra-core:release/1.10 Jun 29, 2018

Penguinwizzard deleted the dev15_prefast_fixes branch June 29, 2018 00:56

		@@ -2158,6 +2158,7 @@ namespace Js
		::Math::DefaultOverflowPolicy();

Address issues picked up by Dev15 code analysis #5272

Address issues picked up by Dev15 code analysis #5272

Conversation

Penguinwizzard commented Jun 6, 2018

boingoing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Penguinwizzard Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing commented Jun 6, 2018

Penguinwizzard commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev commented Jun 6, 2018

dilijev commented Jun 6, 2018

dilijev commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Penguinwizzard Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev left a comment

Choose a reason for hiding this comment

dilijev Jun 6, 2018 •

edited

Loading

Penguinwizzard Jun 6, 2018 •

edited

Loading

dilijev Jun 6, 2018 •

edited

Loading

MSLaguana commented Jun 7, 2018 •

edited

Loading

Penguinwizzard Jun 8, 2018 •

edited

Loading