Skip to content

Commit 8ac3acc

Browse files
committed
[MERGE #5484 @VSadov] Fix a possible loss of property in the TypePath
Merge pull request #5484 from VSadov:fix17745531 In a rare case TinyDictionary could lose the last value added. We use the sign bit to tag last values in bucket chains. We also use `FF` for uninitialized/empty buckets. In extreme rare cases 127th pid to insert may hash into a a bucket still unused by the previous 126 values. Once 127 is tagged it becomes `FF` which will look like an empty bucket - the 127 value would be lost. The fix is simple - detect the case as described and make 127 to chain to `FF` marker. In such case it will not be the last in the chain and will not be tagged. Fixes: OS:#17745531
2 parents 0a1d70b + d54ac08 commit 8ac3acc

File tree

3 files changed

+58
-2
lines changed

3 files changed

+58
-2
lines changed

lib/Runtime/Types/TypePath.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,12 @@ namespace Js
4343
uint32 bucketIndex = ReduceKeyToIndex(key);
4444

4545
byte i = buckets[bucketIndex];
46-
if (i == NIL)
46+
47+
// if the bucket was empty (NIL), put a tagged value to indicate the end of the chain.
48+
// NB: [OS:17745531] In extreme rare cases 127th pid to insert hashes into a a bucket still unused by the previous 126 values.
49+
// We cannot tag 127 since it would become NIL and we would lose the value.
50+
// We can however chain 127 --> NIL, since it is the same as having two 127 in the bucket, which is ok.
51+
if ((i == NIL) & (value != 127))
4752
{
4853
// set the highest bit to mark the value as the last in the chain
4954
buckets[bucketIndex] = value | 128;

test/Bugs/OS_17745531.js

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
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+
var obj = {};
7+
8+
// create enough pids to pick from
9+
for (let i = 0; i < 20000; i++)
10+
{
11+
Object.defineProperty(obj, 'prop' + i, {
12+
value: i,
13+
writable: true
14+
});
15+
}
16+
17+
for (let j = 0; j < 127; j++)
18+
{
19+
20+
var obj1 = {};
21+
// fill with pids that prone to collisions - to have some empty buckets when inserting 127th property
22+
for (let i = 0; i < 127; i++)
23+
{
24+
Object.defineProperty(obj1, 'prop' + i * 144, {
25+
value: i,
26+
writable: true
27+
});
28+
}
29+
30+
// hopefully 'prop<j>' will hash into an empty bucket and it is also a 127th property.
31+
// we will try multiple j - just in case the empty bucket moves due to minor changes in
32+
// hashing or how pids are assigned.
33+
Object.defineProperty(obj1, 'prop' + j, {
34+
value: obj['prop' + j],
35+
writable: true
36+
});
37+
38+
39+
// compare the values, also keeps objects alive
40+
if (obj['prop' + j] != obj1['prop' + j])
41+
{
42+
console.log("fail");
43+
}
44+
}
45+
46+
console.log("pass");

test/Bugs/rlexe.xml

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -505,7 +505,12 @@
505505
<default>
506506
<files>bug_OS17614914.js</files>
507507
</default>
508-
</test>
508+
</test>
509+
<test>
510+
<default>
511+
<files>OS_17745531.js</files>
512+
</default>
513+
</test>
509514
<test>
510515
<default>
511516
<files>SuperUndoDeferIssue.js</files>

0 commit comments

Comments
 (0)