From dad2c9fadd78d75c193250bbec69abdbe836d11a Mon Sep 17 00:00:00 2001 From: Oguz Bastemur Date: Wed, 14 Feb 2018 14:56:36 -0800 Subject: [PATCH 1/2] ch: use copy instead of direct sourmap cache In case we load the same cached source file twice, ch won't track that and free the buffer twice. --- bin/ch/Helpers.cpp | 160 +++++++++++++++++++++++------------------ bin/ch/WScriptJsrt.cpp | 52 ++++---------- 2 files changed, 103 insertions(+), 109 deletions(-) diff --git a/bin/ch/Helpers.cpp b/bin/ch/Helpers.cpp index 941209f4b81..a07fd3e4ac6 100644 --- a/bin/ch/Helpers.cpp +++ b/bin/ch/Helpers.cpp @@ -187,9 +187,11 @@ HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UIN HRESULT hr = S_OK; BYTE * pRawBytes = nullptr; + BYTE * pRawBytesFromMap = nullptr; UINT lengthBytes = 0; contents = nullptr; FILE * file = NULL; + size_t bufferLength = 0; LPCSTR filename = filenameToLoad; if (sHostApplicationPathBufferLength == (uint)-1) @@ -225,11 +227,8 @@ HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UIN if (SourceMap::Find(filenameToLoad, strlen(filenameToLoad), &data) || SourceMap::Find(filename, strlen(filename), &data)) { - contents = data->GetString(); - if (lengthBytesOut != nullptr) - { - *lengthBytesOut = (UINT) data->GetLength(); - } + pRawBytesFromMap = (BYTE*) data->GetString(); + lengthBytes = (UINT) data->GetLength(); } else { @@ -263,88 +262,109 @@ HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UIN IfFailGo(E_FAIL); } + } - if (file != NULL) - { - // Determine the file length, in bytes. - fseek(file, 0, SEEK_END); - lengthBytes = ftell(file); - fseek(file, 0, SEEK_SET); - const size_t bufferLength = lengthBytes + sizeof(BYTE); - pRawBytes = (LPBYTE)malloc(bufferLength); - if (nullptr == pRawBytes) - { - fwprintf(stderr, _u("out of memory")); - IfFailGo(E_OUTOFMEMORY); - } + if (file != NULL) + { + // Determine the file length, in bytes. + fseek(file, 0, SEEK_END); + lengthBytes = ftell(file); + fseek(file, 0, SEEK_SET); + } - // - // Read the entire content as a binary block. - // - size_t readBytes = fread(pRawBytes, sizeof(BYTE), lengthBytes, file); - if (readBytes < lengthBytes * sizeof(BYTE)) - { - IfFailGo(E_FAIL); - } + if (lengthBytes != 0) + { + bufferLength = lengthBytes + sizeof(BYTE); + pRawBytes = (LPBYTE)malloc(bufferLength); + } - pRawBytes[lengthBytes] = 0; // Null terminate it. Could be UTF16 + if (nullptr == pRawBytes) + { + fwprintf(stderr, _u("out of memory")); + IfFailGo(E_OUTOFMEMORY); + } - // - // Read encoding to make sure it's supported - // - // Warning: The UNICODE buffer for parsing is supposed to be provided by the host. - // This is not a complete read of the encoding. Some encodings like UTF7, UTF1, EBCDIC, SCSU, BOCU could be - // wrongly classified as ANSI - // - { - #pragma warning(push) - // suppressing prefast warning that "readable size is bufferLength bytes but 2 may be read" as bufferLength is clearly > 2 in the code that follows - #pragma warning(disable:6385) - C_ASSERT(sizeof(WCHAR) == 2); - if (bufferLength > 2) - { - __analysis_assume(bufferLength > 2); - #pragma prefast(push) - #pragma prefast(disable:6385, "PREfast incorrectly reports this as an out-of-bound access."); - if ((pRawBytes[0] == 0xFE && pRawBytes[1] == 0xFF) || - (pRawBytes[0] == 0xFF && pRawBytes[1] == 0xFE) || - (bufferLength > 4 && pRawBytes[0] == 0x00 && pRawBytes[1] == 0x00 && - ((pRawBytes[2] == 0xFE && pRawBytes[3] == 0xFF) || - (pRawBytes[2] == 0xFF && pRawBytes[3] == 0xFE)))) - - { - // unicode unsupported - fwprintf(stderr, _u("unsupported file encoding. Only ANSI and UTF8 supported")); - IfFailGo(E_UNEXPECTED); - } - #pragma prefast(pop) - } - } - #pragma warning(pop) + if (file != NULL) + { + // + // Read the entire content as a binary block. + // + size_t readBytes = fread(pRawBytes, sizeof(BYTE), lengthBytes, file); + if (readBytes < lengthBytes * sizeof(BYTE)) + { + IfFailGo(E_FAIL); } + } + else // from module source register + { + // Q: module source is on persistent memory. Why do we use the copy instead? + // A: if we use the same memory twice, ch doesn't know that during FinalizeCallback free. + // the copy memory will be freed by the finalizer + Assert(pRawBytesFromMap); + memcpy_s(pRawBytes, bufferLength, pRawBytesFromMap, lengthBytes); + } - contents = reinterpret_cast(pRawBytes); + if (pRawBytes) + { + pRawBytes[lengthBytes] = 0; // Null terminate it. Could be UTF16 + } - Error: - if (SUCCEEDED(hr)) + if (file != NULL) + { + // + // Read encoding to make sure it's supported + // + // Warning: The UNICODE buffer for parsing is supposed to be provided by the host. + // This is not a complete read of the encoding. Some encodings like UTF7, UTF1, EBCDIC, SCSU, BOCU could be + // wrongly classified as ANSI + // +#pragma warning(push) + // suppressing prefast warning that "readable size is bufferLength + // bytes but 2 may be read" as bufferLength is clearly > 2 in the code that follows +#pragma warning(disable:6385) + C_ASSERT(sizeof(WCHAR) == 2); + if (bufferLength > 2) { - if (lengthBytesOut) + __analysis_assume(bufferLength > 2); +#pragma prefast(push) +#pragma prefast(disable:6385, "PREfast incorrectly reports this as an out-of-bound access."); + if ((pRawBytes[0] == 0xFE && pRawBytes[1] == 0xFF) || + (pRawBytes[0] == 0xFF && pRawBytes[1] == 0xFE) || + (bufferLength > 4 && pRawBytes[0] == 0x00 && pRawBytes[1] == 0x00 && + ((pRawBytes[2] == 0xFE && pRawBytes[3] == 0xFF) || + (pRawBytes[2] == 0xFF && pRawBytes[3] == 0xFE)))) + { - *lengthBytesOut = lengthBytes; + // unicode unsupported + fwprintf(stderr, _u("unsupported file encoding. Only ANSI and UTF8 supported")); + IfFailGo(E_UNEXPECTED); } +#pragma prefast(pop) } +#pragma warning(pop) + } - if (file != NULL) - { - fclose(file); - } + contents = reinterpret_cast(pRawBytes); - if (pRawBytes && reinterpret_cast(pRawBytes) != contents) +Error: + if (SUCCEEDED(hr)) + { + if (lengthBytesOut) { - free(pRawBytes); + *lengthBytesOut = lengthBytes; } } + if (file != NULL) + { + fclose(file); + } + + if (pRawBytes && reinterpret_cast(pRawBytes) != contents) + { + free(pRawBytes); + } + return hr; } diff --git a/bin/ch/WScriptJsrt.cpp b/bin/ch/WScriptJsrt.cpp index f17afbb5b2c..77d0583ec62 100644 --- a/bin/ch/WScriptJsrt.cpp +++ b/bin/ch/WScriptJsrt.cpp @@ -202,15 +202,9 @@ JsValueRef WScriptJsrt::LoadScriptFileHelper(JsValueRef callee, JsValueRef *argu if (FAILED(hr)) { // check if have it registered - AutoString *data; - if (!SourceMap::Find(fileName, &data)) - { - fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString()); - IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue)); - return returnValue; - } - - fileContent = data->GetString(); + fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString()); + IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue)); + return returnValue; } returnValue = LoadScript(callee, *fileName, fileContent, *scriptInjectType ? *scriptInjectType : "self", isSourceModule, WScriptJsrt::FinalizeFree, true); @@ -1104,16 +1098,9 @@ JsValueRef __stdcall WScriptJsrt::LoadTextFileCallback(JsValueRef callee, bool i if (FAILED(hr)) { // check if have it registered - AutoString *data; - if (!SourceMap::Find(fileName, &data)) - { - fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString()); - IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue)); - return returnValue; - } - - fileContent = data->GetString(); - lengthBytes = (UINT) data->GetLength(); + fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString()); + IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue)); + return returnValue; } IfJsrtErrorSetGo(ChakraRTInterface::JsCreateString( @@ -1157,17 +1144,9 @@ JsValueRef __stdcall WScriptJsrt::LoadBinaryFileCallback(JsValueRef callee, if (FAILED(hr)) { // check if have it registered - AutoString *data; - if (!SourceMap::Find(fileName, &data)) - { - fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString()); - IfJsrtErrorSetGoLabel(ChakraRTInterface::JsGetUndefinedValue(&returnValue), Error); - return returnValue; - } - - isHeapAlloc = false; - fileContent = data->GetString(); - lengthBytes = (UINT) data->GetLength(); + fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString()); + IfJsrtErrorSetGoLabel(ChakraRTInterface::JsGetUndefinedValue(&returnValue), Error); + return returnValue; } JsValueRef arrayBuffer; @@ -1607,17 +1586,12 @@ HRESULT WScriptJsrt::ModuleMessage::Call(LPCSTR fileName) if (FAILED(hr)) { // check if have it registered - AutoString *data; - if (!SourceMap::Find(specifierStr, &data)) + if (!HostConfigFlags::flags.MuteHostErrorMsgIsEnabled) { - if (!HostConfigFlags::flags.MuteHostErrorMsgIsEnabled) - { - fprintf(stderr, "Couldn't load file '%s'\n", specifierStr.GetString()); - } - LoadScript(nullptr, *specifierStr, nullptr, "module", true, WScriptJsrt::FinalizeFree, false); - goto Error; + fprintf(stderr, "Couldn't load file '%s'\n", specifierStr.GetString()); } - fileContent = data->GetString(); + LoadScript(nullptr, *specifierStr, nullptr, "module", true, WScriptJsrt::FinalizeFree, false); + goto Error; } LoadScript(nullptr, *specifierStr, fileContent, "module", true, WScriptJsrt::FinalizeFree, true); } From 9df05b29ccf1706f80908333141455cbdd402725 Mon Sep 17 00:00:00 2001 From: Oguz Bastemur Date: Thu, 15 Feb 2018 14:28:06 -0800 Subject: [PATCH 2/2] module: test load twice --- test/es6module/module-load-twice.js | 12 ++++++++++++ test/es6module/rlexe.xml | 5 +++++ 2 files changed, 17 insertions(+) create mode 100755 test/es6module/module-load-twice.js diff --git a/test/es6module/module-load-twice.js b/test/es6module/module-load-twice.js new file mode 100755 index 00000000000..3e01960444b --- /dev/null +++ b/test/es6module/module-load-twice.js @@ -0,0 +1,12 @@ +//------------------------------------------------------------------------------------------------------- +// Copyright (C) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information. +//------------------------------------------------------------------------------------------------------- + +WScript.RegisterModuleSource('a.js', " "); +WScript.LoadScriptFile('a.js'); +WScript.LoadScriptFile('a.js'); +WScript.RegisterModuleSource('b.js', "import * as foo from 'a.js'"); +WScript.LoadScriptFile('b.js', "module"); +WScript.LoadScriptFile('b.js', "module"); +print('pass'); \ No newline at end of file diff --git a/test/es6module/rlexe.xml b/test/es6module/rlexe.xml index 7a3b2d74dea..68e265d3839 100644 --- a/test/es6module/rlexe.xml +++ b/test/es6module/rlexe.xml @@ -141,4 +141,9 @@ BugFix,exclude_sanitize_address + + + module-load-twice.js + +