Skip to content

Commit 59dd3af

Browse files
obastemurobastemur
committed
[MERGE #4683 @obastemur] ch: use copy instead of direct sourmap 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
2 parents 6b3604e + 9df05b2 commit 59dd3af

File tree

4 files changed

+120
-109
lines changed

4 files changed

+120
-109
lines changed

‎bin/ch/Helpers.cpp

Lines changed: 90 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -187,9 +187,11 @@ HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UIN
187187

188188
HRESULT hr = S_OK;
189189
BYTE * pRawBytes = nullptr;
190+
BYTE * pRawBytesFromMap = nullptr;
190191
UINT lengthBytes = 0;
191192
contents = nullptr;
192193
FILE * file = NULL;
194+
size_t bufferLength = 0;
193195

194196
LPCSTR filename = filenameToLoad;
195197
if (sHostApplicationPathBufferLength == (uint)-1)
@@ -225,11 +227,8 @@ HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UIN
225227
if (SourceMap::Find(filenameToLoad, strlen(filenameToLoad), &data) ||
226228
SourceMap::Find(filename, strlen(filename), &data))
227229
{
228-
contents = data->GetString();
229-
if (lengthBytesOut != nullptr)
230-
{
231-
*lengthBytesOut = (UINT) data->GetLength();
232-
}
230+
pRawBytesFromMap = (BYTE*) data->GetString();
231+
lengthBytes = (UINT) data->GetLength();
233232
}
234233
else
235234
{
@@ -263,88 +262,109 @@ HRESULT Helpers::LoadScriptFromFile(LPCSTR filenameToLoad, LPCSTR& contents, UIN
263262

264263
IfFailGo(E_FAIL);
265264
}
265+
}
266266

267-
if (file != NULL)
268-
{
269-
// Determine the file length, in bytes.
270-
fseek(file, 0, SEEK_END);
271-
lengthBytes = ftell(file);
272-
fseek(file, 0, SEEK_SET);
273-
const size_t bufferLength = lengthBytes + sizeof(BYTE);
274-
pRawBytes = (LPBYTE)malloc(bufferLength);
275-
if (nullptr == pRawBytes)
276-
{
277-
fwprintf(stderr, _u("out of memory"));
278-
IfFailGo(E_OUTOFMEMORY);
279-
}
267+
if (file != NULL)
268+
{
269+
// Determine the file length, in bytes.
270+
fseek(file, 0, SEEK_END);
271+
lengthBytes = ftell(file);
272+
fseek(file, 0, SEEK_SET);
273+
}
280274

281-
//
282-
// Read the entire content as a binary block.
283-
//
284-
size_t readBytes = fread(pRawBytes, sizeof(BYTE), lengthBytes, file);
285-
if (readBytes < lengthBytes * sizeof(BYTE))
286-
{
287-
IfFailGo(E_FAIL);
288-
}
275+
if (lengthBytes != 0)
276+
{
277+
bufferLength = lengthBytes + sizeof(BYTE);
278+
pRawBytes = (LPBYTE)malloc(bufferLength);
279+
}
289280

290-
pRawBytes[lengthBytes] = 0; // Null terminate it. Could be UTF16
281+
if (nullptr == pRawBytes)
282+
{
283+
fwprintf(stderr, _u("out of memory"));
284+
IfFailGo(E_OUTOFMEMORY);
285+
}
291286

292-
//
293-
// Read encoding to make sure it's supported
294-
//
295-
// Warning: The UNICODE buffer for parsing is supposed to be provided by the host.
296-
// This is not a complete read of the encoding. Some encodings like UTF7, UTF1, EBCDIC, SCSU, BOCU could be
297-
// wrongly classified as ANSI
298-
//
299-
{
300-
#pragma warning(push)
301-
// suppressing prefast warning that "readable size is bufferLength bytes but 2 may be read" as bufferLength is clearly > 2 in the code that follows
302-
#pragma warning(disable:6385)
303-
C_ASSERT(sizeof(WCHAR) == 2);
304-
if (bufferLength > 2)
305-
{
306-
__analysis_assume(bufferLength > 2);
307-
#pragma prefast(push)
308-
#pragma prefast(disable:6385, "PREfast incorrectly reports this as an out-of-bound access.");
309-
if ((pRawBytes[0] == 0xFE && pRawBytes[1] == 0xFF) ||
310-
(pRawBytes[0] == 0xFF && pRawBytes[1] == 0xFE) ||
311-
(bufferLength > 4 && pRawBytes[0] == 0x00 && pRawBytes[1] == 0x00 &&
312-
((pRawBytes[2] == 0xFE && pRawBytes[3] == 0xFF) ||
313-
(pRawBytes[2] == 0xFF && pRawBytes[3] == 0xFE))))
314-
315-
{
316-
// unicode unsupported
317-
fwprintf(stderr, _u("unsupported file encoding. Only ANSI and UTF8 supported"));
318-
IfFailGo(E_UNEXPECTED);
319-
}
320-
#pragma prefast(pop)
321-
}
322-
}
323-
#pragma warning(pop)
287+
if (file != NULL)
288+
{
289+
//
290+
// Read the entire content as a binary block.
291+
//
292+
size_t readBytes = fread(pRawBytes, sizeof(BYTE), lengthBytes, file);
293+
if (readBytes < lengthBytes * sizeof(BYTE))
294+
{
295+
IfFailGo(E_FAIL);
324296
}
297+
}
298+
else // from module source register
299+
{
300+
// Q: module source is on persistent memory. Why do we use the copy instead?
301+
// A: if we use the same memory twice, ch doesn't know that during FinalizeCallback free.
302+
// the copy memory will be freed by the finalizer
303+
Assert(pRawBytesFromMap);
304+
memcpy_s(pRawBytes, bufferLength, pRawBytesFromMap, lengthBytes);
305+
}
325306

326-
contents = reinterpret_cast<LPCSTR>(pRawBytes);
307+
if (pRawBytes)
308+
{
309+
pRawBytes[lengthBytes] = 0; // Null terminate it. Could be UTF16
310+
}
327311

328-
Error:
329-
if (SUCCEEDED(hr))
312+
if (file != NULL)
313+
{
314+
//
315+
// Read encoding to make sure it's supported
316+
//
317+
// Warning: The UNICODE buffer for parsing is supposed to be provided by the host.
318+
// This is not a complete read of the encoding. Some encodings like UTF7, UTF1, EBCDIC, SCSU, BOCU could be
319+
// wrongly classified as ANSI
320+
//
321+
#pragma warning(push)
322+
// suppressing prefast warning that "readable size is bufferLength
323+
// bytes but 2 may be read" as bufferLength is clearly > 2 in the code that follows
324+
#pragma warning(disable:6385)
325+
C_ASSERT(sizeof(WCHAR) == 2);
326+
if (bufferLength > 2)
330327
{
331-
if (lengthBytesOut)
328+
__analysis_assume(bufferLength > 2);
329+
#pragma prefast(push)
330+
#pragma prefast(disable:6385, "PREfast incorrectly reports this as an out-of-bound access.");
331+
if ((pRawBytes[0] == 0xFE && pRawBytes[1] == 0xFF) ||
332+
(pRawBytes[0] == 0xFF && pRawBytes[1] == 0xFE) ||
333+
(bufferLength > 4 && pRawBytes[0] == 0x00 && pRawBytes[1] == 0x00 &&
334+
((pRawBytes[2] == 0xFE && pRawBytes[3] == 0xFF) ||
335+
(pRawBytes[2] == 0xFF && pRawBytes[3] == 0xFE))))
336+
332337
{
333-
*lengthBytesOut = lengthBytes;
338+
// unicode unsupported
339+
fwprintf(stderr, _u("unsupported file encoding. Only ANSI and UTF8 supported"));
340+
IfFailGo(E_UNEXPECTED);
334341
}
342+
#pragma prefast(pop)
335343
}
344+
#pragma warning(pop)
345+
}
336346

337-
if (file != NULL)
338-
{
339-
fclose(file);
340-
}
347+
contents = reinterpret_cast<LPCSTR>(pRawBytes);
341348

342-
if (pRawBytes && reinterpret_cast<LPCSTR>(pRawBytes) != contents)
349+
Error:
350+
if (SUCCEEDED(hr))
351+
{
352+
if (lengthBytesOut)
343353
{
344-
free(pRawBytes);
354+
*lengthBytesOut = lengthBytes;
345355
}
346356
}
347357

358+
if (file != NULL)
359+
{
360+
fclose(file);
361+
}
362+
363+
if (pRawBytes && reinterpret_cast<LPCSTR>(pRawBytes) != contents)
364+
{
365+
free(pRawBytes);
366+
}
367+
348368
return hr;
349369
}
350370

‎bin/ch/WScriptJsrt.cpp

Lines changed: 13 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -202,15 +202,9 @@ JsValueRef WScriptJsrt::LoadScriptFileHelper(JsValueRef callee, JsValueRef *argu
202202
if (FAILED(hr))
203203
{
204204
// check if have it registered
205-
AutoString *data;
206-
if (!SourceMap::Find(fileName, &data))
207-
{
208-
fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString());
209-
IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue));
210-
return returnValue;
211-
}
212-
213-
fileContent = data->GetString();
205+
fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString());
206+
IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue));
207+
return returnValue;
214208
}
215209

216210
returnValue = LoadScript(callee, *fileName, fileContent, *scriptInjectType ? *scriptInjectType : "self", isSourceModule, WScriptJsrt::FinalizeFree, true);
@@ -1104,16 +1098,9 @@ JsValueRef __stdcall WScriptJsrt::LoadTextFileCallback(JsValueRef callee, bool i
11041098
if (FAILED(hr))
11051099
{
11061100
// check if have it registered
1107-
AutoString *data;
1108-
if (!SourceMap::Find(fileName, &data))
1109-
{
1110-
fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString());
1111-
IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue));
1112-
return returnValue;
1113-
}
1114-
1115-
fileContent = data->GetString();
1116-
lengthBytes = (UINT) data->GetLength();
1101+
fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString());
1102+
IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&returnValue));
1103+
return returnValue;
11171104
}
11181105

11191106
IfJsrtErrorSetGo(ChakraRTInterface::JsCreateString(
@@ -1157,17 +1144,9 @@ JsValueRef __stdcall WScriptJsrt::LoadBinaryFileCallback(JsValueRef callee,
11571144
if (FAILED(hr))
11581145
{
11591146
// check if have it registered
1160-
AutoString *data;
1161-
if (!SourceMap::Find(fileName, &data))
1162-
{
1163-
fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString());
1164-
IfJsrtErrorSetGoLabel(ChakraRTInterface::JsGetUndefinedValue(&returnValue), Error);
1165-
return returnValue;
1166-
}
1167-
1168-
isHeapAlloc = false;
1169-
fileContent = data->GetString();
1170-
lengthBytes = (UINT) data->GetLength();
1147+
fprintf(stderr, "Couldn't load file '%s'\n", fileName.GetString());
1148+
IfJsrtErrorSetGoLabel(ChakraRTInterface::JsGetUndefinedValue(&returnValue), Error);
1149+
return returnValue;
11711150
}
11721151

11731152
JsValueRef arrayBuffer;
@@ -1607,17 +1586,12 @@ HRESULT WScriptJsrt::ModuleMessage::Call(LPCSTR fileName)
16071586
if (FAILED(hr))
16081587
{
16091588
// check if have it registered
1610-
AutoString *data;
1611-
if (!SourceMap::Find(specifierStr, &data))
1589+
if (!HostConfigFlags::flags.MuteHostErrorMsgIsEnabled)
16121590
{
1613-
if (!HostConfigFlags::flags.MuteHostErrorMsgIsEnabled)
1614-
{
1615-
fprintf(stderr, "Couldn't load file '%s'\n", specifierStr.GetString());
1616-
}
1617-
LoadScript(nullptr, *specifierStr, nullptr, "module", true, WScriptJsrt::FinalizeFree, false);
1618-
goto Error;
1591+
fprintf(stderr, "Couldn't load file '%s'\n", specifierStr.GetString());
16191592
}
1620-
fileContent = data->GetString();
1593+
LoadScript(nullptr, *specifierStr, nullptr, "module", true, WScriptJsrt::FinalizeFree, false);
1594+
goto Error;
16211595
}
16221596
LoadScript(nullptr, *specifierStr, fileContent, "module", true, WScriptJsrt::FinalizeFree, true);
16231597
}

‎test/es6module/module-load-twice.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
WScript.RegisterModuleSource('a.js', " ");
7+
WScript.LoadScriptFile('a.js');
8+
WScript.LoadScriptFile('a.js');
9+
WScript.RegisterModuleSource('b.js', "import * as foo from 'a.js'");
10+
WScript.LoadScriptFile('b.js', "module");
11+
WScript.LoadScriptFile('b.js', "module");
12+
print('pass');

‎test/es6module/rlexe.xml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,4 +141,9 @@
141141
<tags>BugFix,exclude_sanitize_address</tags>
142142
</default>
143143
</test>
144+
<test>
145+
<default>
146+
<files>module-load-twice.js</files>
147+
</default>
148+
</test>
144149
</regress-exe>

0 commit comments

Comments
 (0)