Correcting memory allocation in LiteralStringWithPropertyStringPtr::NewFromCString

MSLaguana · MSLaguana · commit 1492900652dd · 2018-02-06T08:57:13.000-08:00
A confusion between bytes and character count meant that JsCreateString
was creating strings that used twice as much space as they needed.
diff --git a/lib/Common/Codex/Utf8Helper.h b/lib/Common/Codex/Utf8Helper.h
@@ -87,7 +87,7 @@ namespace utf8
     }
 
     inline HRESULT NarrowStringToWideNoAlloc(_In_ LPCSTR sourceString, size_t sourceCount,
-        __out_ecount(destBufferCount) LPWSTR destString, size_t destBufferCount, _Out_ size_t* destCount)
+        __out_ecount(destBufferCount) LPWSTR destString, size_t destBufferCount, _Out_ charcount_t* destCount)
     {
         size_t sourceStart = 0;
         size_t cbSourceString = sourceCount;
@@ -123,7 +123,7 @@ namespace utf8
 
         if (sourceStart == sourceCount)
         {
-            *destCount = sourceCount;
+            *destCount = static_cast<charcount_t>(sourceCount);
             destString[sourceCount] = WCHAR(0);
         }
         else
@@ -160,7 +160,7 @@ namespace utf8
     ///
     template <typename AllocatorFunction>
     HRESULT NarrowStringToWide(_In_ AllocatorFunction allocator,_In_ LPCSTR sourceString,
-        size_t sourceCount, _Out_ LPWSTR* destStringPtr, _Out_ size_t* destCount, size_t* allocateCount = nullptr)
+        size_t sourceCount, _Out_ LPWSTR* destStringPtr, _Out_ charcount_t* destCount, size_t* allocateCount = nullptr)
     {
         size_t cbDestString = (sourceCount + 1) * sizeof(WCHAR);
         if (cbDestString < sourceCount) // overflow ?
@@ -184,7 +184,7 @@ namespace utf8
     }
 
     template <class Allocator>
-    HRESULT NarrowStringToWide(_In_ LPCSTR sourceString, size_t sourceCount, _Out_ LPWSTR* destStringPtr, _Out_ size_t* destCount, size_t* allocateCount = nullptr)
+    HRESULT NarrowStringToWide(_In_ LPCSTR sourceString, size_t sourceCount, _Out_ LPWSTR* destStringPtr, _Out_ charcount_t* destCount, size_t* allocateCount = nullptr)
     {
         return NarrowStringToWide(Allocator::allocate, sourceString, sourceCount, destStringPtr, destCount, allocateCount);
     }
@@ -205,30 +205,30 @@ namespace utf8
 
     inline HRESULT NarrowStringToWideDynamic(_In_ LPCSTR sourceString, _Out_ LPWSTR* destStringPtr)
     {
-        size_t unused;
+        charcount_t unused;
         return NarrowStringToWide<malloc_allocator>(
             sourceString, strlen(sourceString), destStringPtr, &unused);
     }
 
-    inline HRESULT NarrowStringToWideDynamicGetLength(_In_ LPCSTR sourceString, _Out_ LPWSTR* destStringPtr, _Out_ size_t* destLength)
+    inline HRESULT NarrowStringToWideDynamicGetLength(_In_ LPCSTR sourceString, _Out_ LPWSTR* destStringPtr, _Out_ charcount_t* destLength)
     {
         return NarrowStringToWide<malloc_allocator>(
             sourceString, strlen(sourceString), destStringPtr, destLength);
     }
 
-    template <class Allocator, class SrcType, class DstType>
+    template <class Allocator, class SrcType, class DstType, class CountType>
     class NarrowWideStringConverter
     {
     public:
         static size_t Length(const SrcType& src);
         static HRESULT Convert(
-            SrcType src, size_t srcCount, DstType* dst, size_t* dstCount, size_t* allocateCount = nullptr);
+            SrcType src, size_t srcCount, DstType* dst, CountType* dstCount, size_t* allocateCount = nullptr);
         static HRESULT ConvertNoAlloc(
-            SrcType src, size_t srcCount, DstType dst, size_t dstCount, size_t* written);
+            SrcType src, size_t srcCount, DstType dst, CountType dstCount, CountType* written);
     };
 
     template <class Allocator>
-    class NarrowWideStringConverter<Allocator, LPCSTR, LPWSTR>
+    class NarrowWideStringConverter<Allocator, LPCSTR, LPWSTR, charcount_t>
     {
     public:
         // Note: Typically caller should pass in Utf8 string length. Following
@@ -240,23 +240,23 @@ namespace utf8
 
         static HRESULT Convert(
             LPCSTR sourceString, size_t sourceCount,
-            LPWSTR* destStringPtr, size_t* destCount, size_t* allocateCount = nullptr)
+            LPWSTR* destStringPtr, charcount_t * destCount, size_t* allocateCount = nullptr)
         {
             return NarrowStringToWide<Allocator>(
                 sourceString, sourceCount, destStringPtr, destCount, allocateCount);
         }
 
         static HRESULT ConvertNoAlloc(
             LPCSTR sourceString, size_t sourceCount,
-            LPWSTR destStringPtr, size_t destCount, size_t* written)
+            LPWSTR destStringPtr, charcount_t destCount, charcount_t* written)
         {
             return NarrowStringToWideNoAlloc(
                 sourceString, sourceCount, destStringPtr, destCount, written);
         }
     };
 
     template <class Allocator>
-    class NarrowWideStringConverter<Allocator, LPCWSTR, LPSTR>
+    class NarrowWideStringConverter<Allocator, LPCWSTR, LPSTR, size_t>
     {
     public:
         // Note: Typically caller should pass in WCHAR string length. Following
@@ -283,14 +283,14 @@ namespace utf8
         }
     };
 
-    template <class Allocator, class SrcType, class DstType>
+    template <class Allocator, class SrcType, class DstType, class CountType>
     class NarrowWideConverter
     {
-        typedef NarrowWideStringConverter<Allocator, SrcType, DstType>
+        typedef NarrowWideStringConverter<Allocator, SrcType, DstType, CountType>
             StringConverter;
     private:
         DstType dst;
-        size_t dstCount;
+        CountType dstCount;
         size_t allocateCount;
         bool freeDst;
 
@@ -347,6 +347,6 @@ namespace utf8
         }
     };
 
-    typedef NarrowWideConverter<malloc_allocator, LPCSTR, LPWSTR> NarrowToWide;
-    typedef NarrowWideConverter<malloc_allocator, LPCWSTR, LPSTR> WideToNarrow;
+    typedef NarrowWideConverter<malloc_allocator, LPCSTR, LPWSTR, charcount_t> NarrowToWide;
+    typedef NarrowWideConverter<malloc_allocator, LPCWSTR, LPSTR, size_t> WideToNarrow;
 }
diff --git a/lib/Jsrt/Jsrt.cpp b/lib/Jsrt/Jsrt.cpp
@@ -4662,7 +4662,7 @@ CHAKRA_API JsCreateString(
         length = strlen(content);
     }
 
-    if (length > static_cast<CharCount>(-1))
+    if (length > MaxCharCount)
     {
         return JsErrorOutOfMemory;
     }
diff --git a/lib/Runtime/Library/ConcatString.cpp b/lib/Runtime/Library/ConcatString.cpp
@@ -99,24 +99,24 @@ namespace Js
         }
 
         ScriptContext * scriptContext = library->GetScriptContext();
-        size_t cbDestString = (charCount + 1) * sizeof(WCHAR);
-        if ((CharCount)cbDestString < charCount) // overflow
+        if (charCount > MaxCharCount)
         {
             Js::JavascriptError::ThrowOutOfMemoryError(scriptContext);
         }
 
         Recycler * recycler = library->GetRecycler();
-        char16* destString = RecyclerNewArrayLeaf(recycler, WCHAR, cbDestString);
+        char16* destString = RecyclerNewArrayLeaf(recycler, WCHAR, charCount + 1);
         if (destString == nullptr)
         {
             Js::JavascriptError::ThrowOutOfMemoryError(scriptContext);
         }
 
-        HRESULT result = utf8::NarrowStringToWideNoAlloc(cString, charCount, destString, charCount + 1, &cbDestString);
+        charcount_t cchDestString = 0;
+        HRESULT result = utf8::NarrowStringToWideNoAlloc(cString, charCount, destString, charCount + 1, &cchDestString);
 
         if (result == S_OK)
         {
-            return (JavascriptString*) RecyclerNew(library->GetRecycler(), LiteralStringWithPropertyStringPtr, destString, (CharCount)cbDestString, library);
+            return (JavascriptString*) RecyclerNew(library->GetRecycler(), LiteralStringWithPropertyStringPtr, destString, cchDestString, library);
         }
 
         Js::JavascriptError::ThrowOutOfMemoryError(scriptContext);
diff --git a/lib/Runtime/Library/WabtInterface.cpp b/lib/Runtime/Library/WabtInterface.cpp
@@ -18,11 +18,11 @@ struct Context
     ScriptContext* scriptContext;
 };
 
-char16* NarrowStringToWide(Context* ctx, const char* src, const size_t* srcSize = nullptr, size_t* dstSize = nullptr)
+char16* NarrowStringToWide(Context* ctx, const char* src, const size_t* srcSize = nullptr, charcount_t* dstSize = nullptr)
 {
     auto allocator = [&ctx](size_t size) {return (char16*)AnewArray(ctx->allocator, char16, size); };
     char16* dst = nullptr;
-    size_t size;
+    charcount_t size;
     HRESULT hr = utf8::NarrowStringToWide(allocator, src, srcSize ? *srcSize : strlen(src), &dst, &size);
     if (hr != S_OK)
     {
@@ -86,11 +86,11 @@ Js::Var Int64ToVar(int64 value, void* user_data)
 Js::Var StringToVar(const char* src, uint length, void* user_data)
 {
     Context* ctx = (Context*)user_data;
-    size_t bufSize = 0;
+    charcount_t bufSize = 0;
     size_t slength = (size_t)length;
     char16* buf = NarrowStringToWide(ctx, src, &slength, &bufSize);
     Assert(bufSize < UINT32_MAX);
-    return JavascriptString::NewCopyBuffer(buf, (charcount_t)bufSize, ctx->scriptContext);
+    return JavascriptString::NewCopyBuffer(buf, bufSize, ctx->scriptContext);
 }
 
 Js::Var CreateBuffer(const uint8* buf, uint size, void* user_data)

Original file line number	Diff line number	Diff line change
`@@ -87,7 +87,7 @@ namespace utf8`
`87`	`87`	`}`
`88`	`88`
`89`	`89`	`inline HRESULT NarrowStringToWideNoAlloc(_In_ LPCSTR sourceString, size_t sourceCount,`
`90`		`- __out_ecount(destBufferCount) LPWSTR destString, size_t destBufferCount, _Out_ size_t* destCount)`
	`90`	`+ __out_ecount(destBufferCount) LPWSTR destString, size_t destBufferCount, _Out_ charcount_t* destCount)`
`91`	`91`	`{`
`92`	`92`	`size_t sourceStart = 0;`
`93`	`93`	`size_t cbSourceString = sourceCount;`
`@@ -123,7 +123,7 @@ namespace utf8`
`123`	`123`
`124`	`124`	`if (sourceStart == sourceCount)`
`125`	`125`	`{`
`126`		`- *destCount = sourceCount;`
	`126`	`+ *destCount = static_cast<charcount_t>(sourceCount);`
`127`	`127`	`destString[sourceCount] = WCHAR(0);`
`128`	`128`	`}`
`129`	`129`	`else`
`@@ -160,7 +160,7 @@ namespace utf8`
`160`	`160`	`///`
`161`	`161`	`template <typename AllocatorFunction>`
`162`	`162`	`HRESULT NarrowStringToWide(_In_ AllocatorFunction allocator,_In_ LPCSTR sourceString,`
`163`		`- size_t sourceCount, _Out_ LPWSTR* destStringPtr, _Out_ size_t* destCount, size_t* allocateCount = nullptr)`
	`163`	`+ size_t sourceCount, _Out_ LPWSTR* destStringPtr, _Out_ charcount_t* destCount, size_t* allocateCount = nullptr)`
`164`	`164`	`{`
`165`	`165`	`size_t cbDestString = (sourceCount + 1) * sizeof(WCHAR);`
`166`	`166`	`if (cbDestString < sourceCount) // overflow ?`
`@@ -184,7 +184,7 @@ namespace utf8`
`184`	`184`	`}`
`185`	`185`
`186`	`186`	`template <class Allocator>`
`187`		`- HRESULT NarrowStringToWide(_In_ LPCSTR sourceString, size_t sourceCount, _Out_ LPWSTR* destStringPtr, _Out_ size_t* destCount, size_t* allocateCount = nullptr)`
	`187`	`+ HRESULT NarrowStringToWide(_In_ LPCSTR sourceString, size_t sourceCount, _Out_ LPWSTR* destStringPtr, _Out_ charcount_t* destCount, size_t* allocateCount = nullptr)`
`188`	`188`	`{`
`189`	`189`	`return NarrowStringToWide(Allocator::allocate, sourceString, sourceCount, destStringPtr, destCount, allocateCount);`
`190`	`190`	`}`
`@@ -205,30 +205,30 @@ namespace utf8`
`205`	`205`
`206`	`206`	`inline HRESULT NarrowStringToWideDynamic(_In_ LPCSTR sourceString, _Out_ LPWSTR* destStringPtr)`
`207`	`207`	`{`
`208`		`- size_t unused;`
	`208`	`+ charcount_t unused;`
`209`	`209`	`return NarrowStringToWide<malloc_allocator>(`
`210`	`210`	`sourceString, strlen(sourceString), destStringPtr, &unused);`
`211`	`211`	`}`
`212`	`212`
`213`		`- inline HRESULT NarrowStringToWideDynamicGetLength(_In_ LPCSTR sourceString, _Out_ LPWSTR* destStringPtr, _Out_ size_t* destLength)`
	`213`	`+ inline HRESULT NarrowStringToWideDynamicGetLength(_In_ LPCSTR sourceString, _Out_ LPWSTR* destStringPtr, _Out_ charcount_t* destLength)`
`214`	`214`	`{`
`215`	`215`	`return NarrowStringToWide<malloc_allocator>(`
`216`	`216`	`sourceString, strlen(sourceString), destStringPtr, destLength);`
`217`	`217`	`}`
`218`	`218`
`219`		`- template <class Allocator, class SrcType, class DstType>`
	`219`	`+ template <class Allocator, class SrcType, class DstType, class CountType>`
`220`	`220`	`class NarrowWideStringConverter`
`221`	`221`	`{`
`222`	`222`	`public:`
`223`	`223`	`static size_t Length(const SrcType& src);`
`224`	`224`	`static HRESULT Convert(`
`225`		`- SrcType src, size_t srcCount, DstType* dst, size_t* dstCount, size_t* allocateCount = nullptr);`
	`225`	`+ SrcType src, size_t srcCount, DstType* dst, CountType* dstCount, size_t* allocateCount = nullptr);`
`226`	`226`	`static HRESULT ConvertNoAlloc(`
`227`		`- SrcType src, size_t srcCount, DstType dst, size_t dstCount, size_t* written);`
	`227`	`+ SrcType src, size_t srcCount, DstType dst, CountType dstCount, CountType* written);`
`228`	`228`	`};`
`229`	`229`
`230`	`230`	`template <class Allocator>`
`231`		`- class NarrowWideStringConverter<Allocator, LPCSTR, LPWSTR>`
	`231`	`+ class NarrowWideStringConverter<Allocator, LPCSTR, LPWSTR, charcount_t>`
`232`	`232`	`{`
`233`	`233`	`public:`
`234`	`234`	`// Note: Typically caller should pass in Utf8 string length. Following`
`@@ -240,23 +240,23 @@ namespace utf8`
`240`	`240`
`241`	`241`	`static HRESULT Convert(`
`242`	`242`	`LPCSTR sourceString, size_t sourceCount,`
`243`		`- LPWSTR* destStringPtr, size_t* destCount, size_t* allocateCount = nullptr)`
	`243`	`+ LPWSTR* destStringPtr, charcount_t * destCount, size_t* allocateCount = nullptr)`
`244`	`244`	`{`
`245`	`245`	`return NarrowStringToWide<Allocator>(`
`246`	`246`	`sourceString, sourceCount, destStringPtr, destCount, allocateCount);`
`247`	`247`	`}`
`248`	`248`
`249`	`249`	`static HRESULT ConvertNoAlloc(`
`250`	`250`	`LPCSTR sourceString, size_t sourceCount,`
`251`		`- LPWSTR destStringPtr, size_t destCount, size_t* written)`
	`251`	`+ LPWSTR destStringPtr, charcount_t destCount, charcount_t* written)`
`252`	`252`	`{`
`253`	`253`	`return NarrowStringToWideNoAlloc(`
`254`	`254`	`sourceString, sourceCount, destStringPtr, destCount, written);`
`255`	`255`	`}`
`256`	`256`	`};`
`257`	`257`
`258`	`258`	`template <class Allocator>`
`259`		`- class NarrowWideStringConverter<Allocator, LPCWSTR, LPSTR>`
	`259`	`+ class NarrowWideStringConverter<Allocator, LPCWSTR, LPSTR, size_t>`
`260`	`260`	`{`
`261`	`261`	`public:`
`262`	`262`	`// Note: Typically caller should pass in WCHAR string length. Following`
`@@ -283,14 +283,14 @@ namespace utf8`
`283`	`283`	`}`
`284`	`284`	`};`
`285`	`285`
`286`		`- template <class Allocator, class SrcType, class DstType>`
	`286`	`+ template <class Allocator, class SrcType, class DstType, class CountType>`
`287`	`287`	`class NarrowWideConverter`
`288`	`288`	`{`
`289`		`- typedef NarrowWideStringConverter<Allocator, SrcType, DstType>`
	`289`	`+ typedef NarrowWideStringConverter<Allocator, SrcType, DstType, CountType>`
`290`	`290`	`StringConverter;`
`291`	`291`	`private:`
`292`	`292`	`DstType dst;`
`293`		`- size_t dstCount;`
	`293`	`+ CountType dstCount;`
`294`	`294`	`size_t allocateCount;`
`295`	`295`	`bool freeDst;`
`296`	`296`
`@@ -347,6 +347,6 @@ namespace utf8`
`347`	`347`	`}`
`348`	`348`	`};`
`349`	`349`
`350`		`- typedef NarrowWideConverter<malloc_allocator, LPCSTR, LPWSTR> NarrowToWide;`
`351`		`- typedef NarrowWideConverter<malloc_allocator, LPCWSTR, LPSTR> WideToNarrow;`
	`350`	`+ typedef NarrowWideConverter<malloc_allocator, LPCSTR, LPWSTR, charcount_t> NarrowToWide;`
	`351`	`+ typedef NarrowWideConverter<malloc_allocator, LPCWSTR, LPSTR, size_t> WideToNarrow;`
`352`	`352`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4662,7 +4662,7 @@ CHAKRA_API JsCreateString(`
`4662`	`4662`	`length = strlen(content);`
`4663`	`4663`	`}`
`4664`	`4664`
`4665`		`- if (length > static_cast<CharCount>(-1))`
	`4665`	`+ if (length > MaxCharCount)`
`4666`	`4666`	`{`
`4667`	`4667`	`return JsErrorOutOfMemory;`
`4668`	`4668`	`}`
Original file line number	Diff line number	Diff line change
`@@ -99,24 +99,24 @@ namespace Js`
`99`	`99`	`}`
`100`	`100`
`101`	`101`	`ScriptContext * scriptContext = library->GetScriptContext();`
`102`		`- size_t cbDestString = (charCount + 1) * sizeof(WCHAR);`
`103`		`- if ((CharCount)cbDestString < charCount) // overflow`
	`102`	`+ if (charCount > MaxCharCount)`
`104`	`103`	`{`
`105`	`104`	`Js::JavascriptError::ThrowOutOfMemoryError(scriptContext);`
`106`	`105`	`}`
`107`	`106`
`108`	`107`	`Recycler * recycler = library->GetRecycler();`
`109`		`- char16* destString = RecyclerNewArrayLeaf(recycler, WCHAR, cbDestString);`
	`108`	`+ char16* destString = RecyclerNewArrayLeaf(recycler, WCHAR, charCount + 1);`
`110`	`109`	`if (destString == nullptr)`
`111`	`110`	`{`
`112`	`111`	`Js::JavascriptError::ThrowOutOfMemoryError(scriptContext);`
`113`	`112`	`}`
`114`	`113`
`115`		`- HRESULT result = utf8::NarrowStringToWideNoAlloc(cString, charCount, destString, charCount + 1, &cbDestString);`
	`114`	`+ charcount_t cchDestString = 0;`
	`115`	`+ HRESULT result = utf8::NarrowStringToWideNoAlloc(cString, charCount, destString, charCount + 1, &cchDestString);`
`116`	`116`
`117`	`117`	`if (result == S_OK)`
`118`	`118`	`{`
`119`		`- return (JavascriptString*) RecyclerNew(library->GetRecycler(), LiteralStringWithPropertyStringPtr, destString, (CharCount)cbDestString, library);`
	`119`	`+ return (JavascriptString*) RecyclerNew(library->GetRecycler(), LiteralStringWithPropertyStringPtr, destString, cchDestString, library);`
`120`	`120`	`}`
`121`	`121`
`122`	`122`	`Js::JavascriptError::ThrowOutOfMemoryError(scriptContext);`