Skip to content

Commit 781e62d

Browse files
committed
permission: make inserting an already-granted path idempotent
`Node::CreateChild` treats an empty remaining prefix as reachable only when the node is not yet a leaf. Inserting a second grant that is identical up to and including its first `*` walks to the existing node, marks it a leaf, and recurses with an empty prefix. The `!is_leaf` condition is then false and the unconditional `CHECK` aborts the process. Startup is unaffected because `GrantAccess` only inserts when the path is not already covered, so the colliding insert is skipped while flags are applied. `RebuildTree` replays the recorded grant list unconditionally, so the first `permission.drop()` performs the insert that startup skipped. Inserting a path that is already granted is a no-op, so return the existing node instead of asserting. Fixes: #66901
1 parent a232998 commit 781e62d

2 files changed

Lines changed: 90 additions & 1 deletion

File tree

‎src/permission/fs_permission.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,9 @@ class FSPermission final : public PermissionBase {
5454
}
5555

5656
Node* CreateChild(std::string_view path_prefix) {
57-
if (path_prefix.empty() && !is_leaf) {
57+
// An empty remaining prefix means this node already represents the
58+
// inserted path. Re-inserting it is a no-op.
59+
if (path_prefix.empty()) {
5860
is_leaf = true;
5961
return this;
6062
}
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
// Flags: --permission --allow-fs-read=* --allow-child-process
2+
'use strict';
3+
4+
// Regression test for https://github.com/nodejs/node/issues/66901
5+
//
6+
// Two --allow-fs-read grants that are identical up to and including their
7+
// first `*` are deduplicated by GrantAccess, so the colliding insert never
8+
// happens while the flags are applied. permission.drop() calls RebuildTree,
9+
// which replays the recorded grant list unconditionally, and the second
10+
// insert reached an unconditional CHECK in Node::CreateChild and aborted
11+
// the process with SIGABRT.
12+
13+
const common = require('../common');
14+
const { isMainThread } = require('worker_threads');
15+
16+
if (!isMainThread) {
17+
common.skip('This test only works on a main thread');
18+
}
19+
20+
const assert = require('assert');
21+
const path = require('path');
22+
const { spawnSync } = require('child_process');
23+
const tmpdir = require('../common/tmpdir');
24+
25+
tmpdir.refresh();
26+
27+
const dir = path.join(tmpdir.path, 'sig');
28+
const script = path.join(dir, 'app.js');
29+
30+
require('fs').mkdirSync(dir, { recursive: true });
31+
require('fs').writeFileSync(
32+
script,
33+
'process.permission.drop("fs.read", process.argv[1]);\n' +
34+
'console.log("survived");\n',
35+
);
36+
37+
{
38+
// Colliding grants plus a drop() must not abort the process.
39+
const { status, signal, stdout, stderr } = spawnSync(
40+
process.execPath,
41+
[
42+
'--permission',
43+
`--allow-fs-read=${path.join(dir, '*')}`,
44+
`--allow-fs-read=${path.join(dir, '*.js')}`,
45+
script,
46+
],
47+
{ encoding: 'utf8' },
48+
);
49+
50+
assert.strictEqual(signal, null, `killed by ${signal}\n${stderr}`);
51+
assert.strictEqual(status, 0, `exited ${status}\n${stderr}`);
52+
assert.match(stdout, /survived/);
53+
}
54+
55+
{
56+
// Control: a single grant with the same drop() already worked, and must
57+
// keep working.
58+
const { status, signal } = spawnSync(
59+
process.execPath,
60+
['--permission', `--allow-fs-read=${path.join(dir, '*')}`, script],
61+
{ encoding: 'utf8' },
62+
);
63+
64+
assert.strictEqual(signal, null);
65+
assert.strictEqual(status, 0);
66+
}
67+
68+
{
69+
// The colliding grants must still grant what they say they grant.
70+
const probe = path.join(dir, 'probe.txt');
71+
require('fs').writeFileSync(probe, 'ok');
72+
73+
const { status, stdout, stderr } = spawnSync(
74+
process.execPath,
75+
[
76+
'--permission',
77+
`--allow-fs-read=${path.join(dir, '*')}`,
78+
`--allow-fs-read=${path.join(dir, '*.js')}`,
79+
'-e',
80+
`console.log(require('fs').readFileSync(${JSON.stringify(probe)}, 'utf8'));`,
81+
],
82+
{ encoding: 'utf8' },
83+
);
84+
85+
assert.strictEqual(status, 0, stderr);
86+
assert.match(stdout, /ok/);
87+
}

0 commit comments

Comments
 (0)