Update with code review feedback

bin/ch/CMakeLists.txt

@@ -5,7 +5,7 @@ add_executable (ch
   Helpers.cpp
   HostConfigFlags.cpp
   WScriptJsrt.cpp
-    )
+  )
 
 include_directories(..)
 

bin/ch/Helpers.cpp

@@ -14,14 +14,22 @@
 HRESULT Helpers::WideStringToNarrowDynamic(LPCWSTR sourceString, LPSTR* destStringPtr)
 {
     size_t cchSourceString = wcslen(sourceString);
-    size_t cbDestString = (cchSourceString + 1) * 3;
     
-    utf8char_t* destString = (utf8char_t*)malloc(cbDestString);
-    if (destString == nullptr)
+    if (cchSourceString >= MAXUINT32)
     {
         return E_OUTOFMEMORY;
     }
-    if (cchSourceString >= MAXUINT32)
+
+    size_t cbDestString = (cchSourceString + 1) * 3;
+
+    // Check for overflow- cbDestString should be >= cchSourceString
+    if (cbDestString < cchSourceString)
+    {
+        return E_OUTOFMEMORY;
+    }
+
+    utf8char_t* destString = (utf8char_t*)malloc(cbDestString);
+    if (destString == nullptr)
     {
         return E_OUTOFMEMORY;
     }
@@ -45,6 +53,12 @@ HRESULT Helpers::NarrowStringToWideDynamic(LPCSTR sourceString, LPWSTR* destStri
     charcount_t cchDestString = utf8::ByteIndexIntoCharacterIndex((LPCUTF8) sourceString, cbSourceString);
     size_t cbDestString = (cchDestString + 1) * sizeof(WCHAR);
     
+    // Check for overflow- cbDestString should be >= cchSourceString
+    if (cbDestString < cchDestString)
+    {
+        return E_OUTOFMEMORY;
+    }
+
     WCHAR* destString = (WCHAR*)malloc(cbDestString);
     if (destString == nullptr)
     {

bin/ch/MessageQueue.h

@@ -58,30 +58,26 @@ public:
     }
 
     // Scan through the sorted list
-    // Insert before the first node that satisfies the compare function
+    // Insert before the first node that satisfies the LessThan function
     // This function maintains the invariant that the list is always sorted
     void Insert(const T& data)
     {
         DListNode<T>* curr = head;
         DListNode<T>* node = new DListNode<T>(data);
-        DListNode<T>* prevOfCurr = nullptr; 
+        DListNode<T>* prev = nullptr; 
 
         // Now, if we have to insert, we have to insert *after* some node
         while (curr != nullptr)
         {
-            prevOfCurr = curr->prev;
-            if (T::Compare(data, curr->data))
+            if (T::LessThan(data, curr->data))
             {
-                InsertAfter(node, prevOfCurr);
-                return;
+                break;
             }
+            prev = curr;
             curr = curr->next;
         }
 
-        // If we reach this point, we didn't find a node that satisfied the predicate
-        // That means, we're either inserting to the end of the list, or the list is empty 
-        // or we're inserting after head (the insert before head case is taken care of in the loop)
-        InsertAfter(node, (prevOfCurr != nullptr ? prevOfCurr->next : head));
+        InsertAfter(node, prev);
     }
 
     T Pop()
@@ -177,7 +173,7 @@ class MessageQueue
             message(message)
         { }
 
-        static bool Compare(const ListEntry& first, const ListEntry& second)
+        static bool LessThan(const ListEntry& first, const ListEntry& second)
         {
             return first.time < second.time;
         }

bin/ch/ch.cpp

@@ -365,7 +365,6 @@ HRESULT ExecuteTest(const char* fileName)
     bool isUtf8 = false;
     LPCOLESTR contentsRaw = nullptr;
     UINT lengthBytes = 0;
-    char* libraryNameUtf8 = nullptr;
 
     JsContextRef context = JS_INVALID_REFERENCE;
 
@@ -452,11 +451,6 @@ Error:
         ChakraRTInterface::JsDisposeRuntime(runtime);
     }
 
-    if (libraryNameUtf8 != nullptr)
-    {
-        free(libraryNameUtf8);
-    }
-    
     _flushall();
 
     return hr;
@@ -522,7 +516,7 @@ int _cdecl wmain(int argc, __in_ecount(argc) LPWSTR argv[])
 
     // The following code is present to make sure we don't load
     // jscript9.dll etc with ch. Since that isn't a concern on non-Windows
-    // builds, it's safe to conditionally comment it out.
+    // builds, it's safe to conditionally compile it out.
 #ifdef _WIN32    
     ATOM lock = ::AddAtom(szChakraCoreLock);
     AssertMsg(lock, "failed to lock chakracore.dll");

lib/CMakeLists.txt

@@ -1,4 +1,4 @@
 add_subdirectory (Common)
 add_subdirectory (Parser)
 add_subdirectory (Runtime)
-add_subdirectory (Jsrt)
+add_subdirectory (Jsrt)

lib/Jsrt/Jsrt.cpp

@@ -2700,11 +2700,6 @@ JsErrorCode RunSerializedScriptCore(const wchar_t *script, TLoadCallback scriptL
 }
 
 #ifdef _WIN32
-template
-JsErrorCode RunSerializedScriptCore<JsSerializedScriptLoadSourceCallback, JsSerializedScriptUnloadCallback>(const wchar_t *script, JsSerializedScriptLoadSourceCallback scriptLoadCallback,
-    JsSerializedScriptUnloadCallback scriptUnloadCallback, unsigned char *buffer, JsSourceContext sourceContext,
-    const wchar_t *sourceUrl, bool parseOnly, JsValueRef *result);
-
 CHAKRA_API JsParseSerializedScript(_In_z_ const wchar_t * script, _In_ unsigned char *buffer, _In_ JsSourceContext sourceContext,
     _In_z_ const wchar_t *sourceUrl, _Out_ JsValueRef * result)
 {

pal/src/misc/bstr.cpp

@@ -133,7 +133,9 @@ STDAPI_(BSTR) SysAllocStringLen(const OLECHAR *psz, UINT len)
 STDAPI_(void) SysFreeString(const OLECHAR* psz)
 {
     if (psz != NULL)
+    {
         HeapFree(GetProcessHeap(), 0, (LPVOID) psz);
+    }
 }
 
 /***
@@ -151,7 +153,9 @@ STDAPI_(void) SysFreeString(const OLECHAR* psz)
 STDAPI_(BSTR) SysAllocString(const OLECHAR* psz)
 {
     if(psz == NULL)
+    {
         return NULL;
-
+    }
+    
     return SysAllocStringLen(psz, (DWORD)PAL_wcslen(psz));
 }