Skip to content

Commit

Permalink
[1.8>1.9] [MERGE #4683 @obastemur] ch: use copy instead of direct sou…
Browse files Browse the repository at this point in the history 
…rmap cache

Merge pull request #4683 from obastemur:fix_m2free

In case we load the same cached source file twice, ch won't track that and free the buffer twice.

Originally reported by OSS-FUZZ OS#15921043
  • Loading branch information
@obastemur
obastemur committed Feb 17, 2018
2 parents ce816ad + 59dd3af commit bd435f7
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 109 deletions.
160 changes: 90 additions & 70 deletions bin/ch/Helpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,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)
Expand Down Expand Up @@ -205,11 +207,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
{
Expand Down Expand Up @@ -243,88 +242,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<LPCSTR>(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<LPCSTR>(pRawBytes);

if (pRawBytes && reinterpret_cast<LPCSTR>(pRawBytes) != contents)
Error:
if (SUCCEEDED(hr))
{
if (lengthBytesOut)
{
free(pRawBytes);
*lengthBytesOut = lengthBytes;
}
}

if (file != NULL)
{
fclose(file);
}

if (pRawBytes && reinterpret_cast<LPCSTR>(pRawBytes) != contents)
{
free(pRawBytes);
}

return hr;
}

Expand Down
52 changes: 13 additions & 39 deletions bin/ch/WScriptJsrt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
12 changes: 12 additions & 0 deletions test/es6module/module-load-twice.js
View file
Original file line number Diff line number Diff line change
@@ -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');
5 changes: 5 additions & 0 deletions test/es6module/rlexe.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,4 +141,9 @@
<tags>BugFix,exclude_sanitize_address</tags>
</default>
</test>
<test>
<default>
<files>module-load-twice.js</files>
</default>
</test>
</regress-exe>

0 comments on commit bd435f7

Please sign in to comment.