test-hash-seed fails #231

Activity

1. targos commented on Jun 24, 2022

   

   targos

   on Jun 24, 2022

   MemberAuthor

   More actions

   So, it seems that the hash seed cannot be guessed anymore? I don't know who to ping about that @nodejs/v8
2. camillobruni commented on Jun 24, 2022

   

   camillobruni

   on Jun 24, 2022

   More actions

   I don't think we have any guarantee on what hashing function / seeds we use internally in V8.
   What's the purpose of this test actually?
3. targos commented on Jun 24, 2022

   

   targos

   on Jun 24, 2022

   MemberAuthor

   More actions

   The test verifies that two Node.js processes do not use the same hash seed.
   It's related to this security release: https://nodejs.org/en/blog/vulnerability/july-2017-security-releases/#node-js-specific-security-flaws
4. bnoordhuis commented on Jun 25, 2022

   

   bnoordhuis

   on Jun 25, 2022

   Member

   More actions

   I didn't look too deeply into this but that test learns the hash seed by observing insertion time into a Set.

   It leans heavily on the observation that set.has(value) was an ersatz compiler barrier to V8. If V8 learned to optimize around has() calls, then that probably throws off the timing enough to stop the algorithm from converging.

   I was going to suggest starting node with --noopt but when I tried that locally, it ran so slow I gave up after 10 minutes.
5. targos commented on Jul 11, 2022

   

   targos

   on Jul 11, 2022

   MemberAuthor

   More actions

   I'm able to make the test pass again with the following changes:

   diff --git a/test/fixtures/guess-hash-seed.js b/test/fixtures/guess-hash-seed.js
   index 8d69cf9249..c6166450b4 100644
   --- a/test/fixtures/guess-hash-seed.js
   +++ b/test/fixtures/guess-hash-seed.js
   @@ -67,9 +67,6 @@ function hash_to_bucket(hash, numBuckets) {
    function time_set_lookup(set, value) {
      const t1 = process.hrtime();
      for (let i = 0; i < 100; i++) {
   -    // annoyingly, SetHas() is JS code and therefore potentially optimizable.
   -    // However, SetHas() looks up the table using native code, and it seems like
   -    // that's sufficient to prevent the optimizer from doing anything?
        set.has(value);
      }
      const t = process.hrtime(t1);
   @@ -78,6 +75,9 @@ function time_set_lookup(set, value) {
      return secs * 1e9 + nanos;
    }
   
   +// Prevent optimization of SetHas().
   +%NeverOptimizeFunction(time_set_lookup);
   +
    // Set with 256 buckets; bucket 0 full, others empty
    const tester_set_buckets = 256;
    const tester_set = new Set();
   diff --git a/test/pummel/test-hash-seed.js b/test/pummel/test-hash-seed.js
   index 274183d8ce..95ad87d4d8 100644
   --- a/test/pummel/test-hash-seed.js
   +++ b/test/pummel/test-hash-seed.js
   @@ -24,7 +24,14 @@ const requiredCallback = common.mustCall((results) => {
      assert.strictEqual(seeds.length, kRepetitions);
    });
   
   -const generateSeed = () => execFilePromise(process.execPath, [targetScript]);
   +function generateSeed() {
   +  return execFilePromise(process.execPath, [
   +    // Needed for %NeverOptimizeFunction.
   +    '--allow-natives-syntax',
   +    targetScript
   +  ]);
   +}
   +
    const subprocesses = [...new Array(kRepetitions)].map(generateSeed);
   
    Promise.all(subprocesses)

   Is that acceptable?
6. camillobruni commented on Jul 11, 2022

   

   camillobruni

   on Jul 11, 2022

   More actions

   I guess that will work.
   However, we generally don't have any guarantees for --allow-natives-syntax and this might break at any point in the future.

   👍React with 👍1Reacted by Michaël Zasso
7. targos commented on Jul 12, 2022

   

   targos

   on Jul 12, 2022

   MemberAuthor

   More actions

   Done, thank you.
8. targos

   closed this as completedon Jul 12, 2022