fs, test: one set of file attributes in Windows prevents fs.fchmod() from changing mode to RW and breaks incomplete test

[vsemozhetbyt]

• Version: 8.0.0 nightly 2017 05 01
• Platform: Windows 7 x64
• Subsystem: fs, libuv?, test

This is a confusing story here, so I am sorry for possibly clumsy wording.

Prehistory

I was launching tests with own test runner, each test with various context several times, and I've found out that parallel/test-fs-chmod.js was OK on the first run and failed on all the next ones. Moreover, this happened not on all the machines.

This turned up to be caused by some combination of a fixture file attributes.

Excursus

See this small attrib doc for context.

The test fails if both -a and -i flags are unset for a fixture file used in the test. This unsetting combination, strangely enough, correlates with this GUI setting:

Screenshot:

How to reproduce

Try to run these commands with repo root as cwd (firstly, we set a combination of the -a and -i flags; then we check the current attributes; then we run the test twice; then we check if the attributes are the same.; --no-deprecation key there is due to cause of #12795).

Commands :

attrib +a +i test/fixtures/a1.js
attrib test/fixtures/a1.js
node --no-deprecation test/parallel/test-fs-chmod.js
node --no-deprecation test/parallel/test-fs-chmod.js
attrib test/fixtures/a1.js

attrib +a -i test/fixtures/a1.js
attrib test/fixtures/a1.js
node --no-deprecation test/parallel/test-fs-chmod.js
node --no-deprecation test/parallel/test-fs-chmod.js
attrib test/fixtures/a1.js

attrib -a +i test/fixtures/a1.js
attrib test/fixtures/a1.js
node --no-deprecation test/parallel/test-fs-chmod.js
node --no-deprecation test/parallel/test-fs-chmod.js
attrib test/fixtures/a1.js

attrib -a -i test/fixtures/a1.js
attrib test/fixtures/a1.js
node --no-deprecation test/parallel/test-fs-chmod.js
node --no-deprecation test/parallel/test-fs-chmod.js
attrib test/fixtures/a1.js

I see this output (some path details stripped, borders added after each iteration):

Output:

/******************************************************************************/
> attrib +a +i test/fixtures/a1.js

> attrib test/fixtures/a1.js
A       I    fixtures/a1.js

> node --no-deprecation test/parallel/test-fs-chmod.js
33060
33060

> node --no-deprecation test/parallel/test-fs-chmod.js
33060
33060

> attrib test/fixtures/a1.js
A       I    fixtures/a1.js
/******************************************************************************/
> attrib +a -i test/fixtures/a1.js

> attrib test/fixtures/a1.js
A            fixtures/a1.js

> node --no-deprecation test/parallel/test-fs-chmod.js
33060
33060

> node --no-deprecation test/parallel/test-fs-chmod.js
33060
33060

> attrib test/fixtures/a1.js
A            fixtures/a1.js
/******************************************************************************/
> attrib -a +i test/fixtures/a1.js

> attrib test/fixtures/a1.js
        I    fixtures/a1.js

> node --no-deprecation test/parallel/test-fs-chmod.js
33060
33060

> node --no-deprecation test/parallel/test-fs-chmod.js
33060
33060

> attrib test/fixtures/a1.js
        I    fixtures/a1.js
/******************************************************************************/
> attrib -a -i test/fixtures/a1.js

> attrib test/fixtures/a1.js
             fixtures/a1.js

> node --no-deprecation test/parallel/test-fs-chmod.js
33060
33060

> node --no-deprecation test/parallel/test-fs-chmod.js
33060
assert.js:557
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^

Error: EPERM: operation not permitted, open 'fixtures/a1.js'

> attrib test/fixtures/a1.js
     R       fixtures/a1.js
/******************************************************************************/

As you can see, in the last case the second test fails and the file mode is changed.

What is going on here?

test/parallel/test-fs-chmod.js changes the mode of fixtures/a.js with fs.chmod() and the mode of fixtures/a1.js with fs.fchmod() twice on Windows: to read-only and back to read-write. In case of fixtures/a.js and fs.chmod(), all is OK even with -a and -i attributes unset (I've checked this). But with fixtures/a1.js and fs.fchmod(), the beforementioned edge case prevents fs.fchmod() from the second change here: the fixtures/a1.js remains read-only after the first test run and fs.open() in append mode fails here on the second test run.

Why the test does not detect this?

This assertion before failed fs.fchmod() call and this assertion after the failed call check the same mode number in the failed case, but as long as this number is not falsy, the second assertion does not throw. On Windows, this number is not falsy in all the cases: it just differs in normal cases and remains the same in the edge case.

What possibly should be done?

1. fs.fchmod() binding should be checked for this edge case. Maybe the bug should be reported upstream for libuv or even for Windows.
2. The assertions in the test should be made stricter.

Unfortunately, I cannot be any more help in this case, as I am not good in this realm. Sorry.

[vsemozhetbyt]

cc @nodejs/fs, @nodejs/testing, @nodejs/platform-windows,

[refack]

@vsemozhetbyt I've added links to the file named in first comment.

[refack]

BTW: I can repro:

c:\code\node$ attrib -a -i test/fixtures/a1.js

c:\code\node$ attrib test/fixtures/a1.js
                   C:\code\node\test\fixtures\a1.js

c:\code\node$ node --no-deprecation test/parallel/test-fs-chmod.js
33060
33060

c:\code\node$ node --no-deprecation test/parallel/test-fs-chmod.js
33060
assert.js:378
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^

Error: EPERM: operation not permitted, open 'd:\code\node\test\fixtures\a1.js'

[refack]

It's not a edge case, it's a real regression. the test didn't detect that

fs.fchmodSync(fd, mode_sync)

Doesn't work on windows.

[vsemozhetbyt]

Simplified reproducible test for Windows:

test.js:

/******************************************************************************/
'use strict';
/******************************************************************************/
const { strictEqual } = require('assert');
const { execSync } = require('child_process');
const { openSync, chmodSync, fchmodSync, statSync, fstatSync } = require('fs');
/******************************************************************************/
const RO = 0o100444; // 33060 0b1000000100100100
const RW = 0o100666; // 33206 0b1000000110110110

const filePath = 'abc.txt'; // or just __filename
const fd = openSync(filePath, 'a');
/******************************************************************************/
execSync(`attrib +a -i ${filePath}`); // the same with '-a +i' or '+a +i'
/******************************************************************************/
chmodSync(filePath, RW);
strictEqual(statSync(filePath).mode, RW);

chmodSync(filePath, RO);
strictEqual(statSync(filePath).mode, RO);

chmodSync(filePath, RW);
strictEqual(statSync(filePath).mode, RW);
/******************************************************************************/
fchmodSync(fd, RW);
strictEqual(fstatSync(fd).mode, RW);

fchmodSync(fd, RO);
strictEqual(fstatSync(fd).mode, RO);

fchmodSync(fd, RW);
strictEqual(fstatSync(fd).mode, RW);
/******************************************************************************/
execSync(`attrib -a -i ${filePath}`);
/******************************************************************************/
chmodSync(filePath, RW);
strictEqual(statSync(filePath).mode, RW);

chmodSync(filePath, RO);
strictEqual(statSync(filePath).mode, RO);

chmodSync(filePath, RW);
strictEqual(statSync(filePath).mode, RW);
/******************************************************************************/
fchmodSync(fd, RW);
strictEqual(fstatSync(fd).mode, RW);

fchmodSync(fd, RO);
strictEqual(fstatSync(fd).mode, RO);

fchmodSync(fd, RW); // This does not work.
strictEqual(fstatSync(fd).mode, RW); // BANG!
/******************************************************************************/

Output:

assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: 33060 === 33206
    at Object.<anonymous> (test.js:52:1)
    at Module._compile (module.js:582:30)
    at Object.Module._extensions..js (module.js:593:10)
    at Module.load (module.js:516:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:618:10)
    at startup (bootstrap_node.js:144:16)
    at bootstrap_node.js:548:3

The last fchmodSync() fails, the file remains read-only.

[refack]

You are playing with fire manipulating __filename
Does this work for a brand new file created in process.env['TEMP']?
I'll just check...

[vsemozhetbyt]

@refack I've made it more abstract, but it still fails for a new file in the cwd.

[refack]

Cut some stuff out

/******************************************************************************/
'use strict';
/******************************************************************************/
const { strictEqual } = require('assert');
const { execSync } = require('child_process');
const { openSync, chmodSync, fchmodSync, fstatSync } = require('fs');
/******************************************************************************/
const RO = 0o100444; // 33060 0b1000000100100100
const RW = 0o100666; // 33206 0b1000000110110110

const tmpFile = `${process.env['TEMP']}\\${Date.now()}gigi.js`;

const fd = openSync(tmpFile, 'a');
/******************************************************************************/
execSync(`attrib -a -i ${tmpFile}`);
/******************************************************************************/
fchmodSync(fd, RO);
const actual2 = fstatSync(fd).mode;
console.log(`${tmpFile} mode: ${actual2}`)
strictEqual(actual2, RO);

fchmodSync(fd, RW); // This does not work.
const actual3 = fstatSync(fd).mode;
console.log(`${tmpFile} mode: ${actual3}`)
strictEqual(actual3, RW); // BANG!
/******************************************************************************/

Now I got #12835 to fail with new files.
The key is execSync(`attrib -a -i ${tmpFile}`);

[vsemozhetbyt]

@refack Well, the plot thickens :)

I've tried to exhaust all sets of attributes. This is the more general test.

test.js:

/******************************************************************************/
'use strict';
/******************************************************************************/
const { strictEqual } = require('assert');
const { execSync } = require('child_process');
const { openSync, chmodSync, fchmodSync, statSync, fstatSync } = require('fs');
/******************************************************************************/
const winFileAttribs = ['a', 'h', 'i', 'r', 's'];
const toggles = { 0: '-', 1: '+' };

const winFileAttribSets =
  (new Array(Object.keys(toggles).length ** winFileAttribs.length))
  .fill(0)
  .map((set, setIndex) => setIndex
                         .toString(2)
                         .padStart(winFileAttribs.length, 0)
                         .split('')
                         .map((num, numIndex) =>
                           `${toggles[num]}${winFileAttribs[numIndex]}`)
                         .join(' '));
/******************************************************************************/
const RO = 0o100444; // 33060 0b1000000100100100
const RW = 0o100666; // 33206 0b1000000110110110

const filePath = 'abc.txt'; // or just __filename
const fd = openSync(filePath, 'a');
/******************************************************************************/
winFileAttribSets.forEach((winFileAttribSet) => {
  process.stdout.write(`${winFileAttribSet}: `);

  try {
    execSync(`attrib ${winFileAttribSet} ${filePath}`);
    chmodSync(filePath, RO);
    strictEqual(statSync(filePath).mode, RO, 'chmodSync -> RO');

    execSync(`attrib ${winFileAttribSet} ${filePath}`);
    chmodSync(filePath, RW);
    strictEqual(statSync(filePath).mode, RW, 'chmodSync -> RW');

    execSync(`attrib ${winFileAttribSet} ${filePath}`);
    fchmodSync(fd, RO);
    strictEqual(fstatSync(fd).mode, RO, 'fchmodSync -> RO');

    execSync(`attrib ${winFileAttribSet} ${filePath}`);
    fchmodSync(fd, RW);
    strictEqual(fstatSync(fd).mode, RW, 'fchmodSync -> RW');

    console.log('OK');
  } catch (err) {
    console.log('not OK');
    console.error('\n', err, '\n');
  }
});
/******************************************************************************/

Output:

-a -h -i -r -s: OK
-a -h -i -r +s: OK
-a -h -i +r -s: not OK

 { AssertionError: fchmodSync -> RW
    at winFileAttribSets.forEach (e:\DOC\prg\js\node\-test\test.js:46:5)
    at Array.forEach (native)
    at Object.<anonymous> (e:\DOC\prg\js\node\-test\test.js:28:19)
    at Module._compile (module.js:582:30)
    at Object.Module._extensions..js (module.js:593:10)
    at Module.load (module.js:516:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:618:10)
    at startup (bootstrap_node.js:144:16)
  name: 'AssertionError',
  actual: 33060,
  expected: 33206,
  operator: '===',
  message: 'fchmodSync -> RW',
  generatedMessage: false }

-a -h -i +r +s: OK
-a -h +i -r -s: OK
-a -h +i -r +s: OK
-a -h +i +r -s: OK
-a -h +i +r +s: OK
-a +h -i -r -s: OK
-a +h -i -r +s: OK
-a +h -i +r -s: OK
-a +h -i +r +s: OK
-a +h +i -r -s: OK
-a +h +i -r +s: OK
-a +h +i +r -s: OK
-a +h +i +r +s: OK
+a -h -i -r -s: OK
+a -h -i -r +s: OK
+a -h -i +r -s: OK
+a -h -i +r +s: OK
+a -h +i -r -s: OK
+a -h +i -r +s: OK
+a -h +i +r -s: OK
+a -h +i +r +s: OK
+a +h -i -r -s: OK
+a +h -i -r +s: OK
+a +h -i +r -s: OK
+a +h -i +r +s: OK
+a +h +i -r -s: OK
+a +h +i -r +s: OK
+a +h +i +r -s: OK
+a +h +i +r +s: OK

[refack]

Need a good debugging session... I'll start a build...

[vsemozhetbyt]

So, it seems only -a -h -i -r -s set of attributes break fchmod. -a -h -i +r -s case is the same, as it becomes -a -h -i -r -s before the fail.

[refack]

So it's probably something with either 0 or a bad bit-mask.

[refack]

This is a trace of the system calls

19:35:14.2105663
	2180
	node.exe
	Read Metadata
	SUCCESS
	QueryBasicInformationFile
	D:\code\node\test\tmp\a1.js
	CreationTime: 04/05/2017 19:35:13
		LastAccessTime: 04/05/2017 19:35:13
		LastWriteTime: 25/04/2017 07:37:43
		ChangeTime: 04/05/2017 19:35:14
		FileAttributes: N
	REFAELUX\refael
19:35:14.2106125
	2180
	node.exe
	Write Metadata
	SUCCESS
	SetBasicInformationFile
	D:\code\node\test\tmp\a1.js
	CreationTime: 04/05/2017 19:35:13
		LastAccessTime: 04/05/2017 19:35:13
		LastWriteTime: 25/04/2017 07:37:43
		ChangeTime: 04/05/2017 19:35:14
		FileAttributes: RN
	REFAELUX\refael
19:35:14.2190395
	2180
	node.exe
	Read Metadata
	SUCCESS
	QueryBasicInformationFile
	D:\code\node\test\tmp\a1.js
	CreationTime: 04/05/2017 19:35:13
		LastAccessTime: 04/05/2017 19:35:13
		LastWriteTime: 25/04/2017 07:37:43
		ChangeTime: 04/05/2017 19:35:14
		FileAttributes: R
	REFAELUX\refael
19:35:14.2191066
	2180
	node.exe
	Write Metadata
	SUCCESS
	SetBasicInformationFile
	D:\code\node\test\tmp\a1.js
	CreationTime: 04/05/2017 19:35:13
		LastAccessTime: 04/05/2017 19:35:13
		LastWriteTime: 25/04/2017 07:37:43
		ChangeTime: 04/05/2017 19:35:14
		FileAttributes: n/a
	REFAELUX\refael
19:35:14.2191973
	2180
	node.exe
	Read Metadata
	BUFFER OVERFLOW
	QueryAllInformationFile
	D:\code\node\test\tmp\a1.js
	CreationTime: 04/05/2017 19:35:13
		LastAccessTime: 04/05/2017 19:35:13
		LastWriteTime: 25/04/2017 07:37:43
		ChangeTime: 04/05/2017 19:35:14
		FileAttributes: R
		AllocationSize: 4,096
		EndOfFile: 1,469
		NumberOfLinks: 1
		DeletePending: False
		Directory: False
		IndexNumber: 0x70000002134f1
		EaSize: 0
		Access: Append Data/Add Subdirectory/Create Pipe Instance
		Write EA
		Read Attributes
		Write Attributes
		Read Control
		Synchronize
		Position: 0
		Mode: Synchronous IO Non-Alert
		AlignmentRequirement: Word
	REFAELUX\refael

[vsemozhetbyt]

@refack Sorry! I was wrong and the test was wrong. I've fixed it now and updated the log. We need to reset the set of file attributes each time to detect the problematic set and what change it disables. New conclusion: only -a -h -i +r -s set is problematic for fchmod, preventing it from setting RW mode (= from removing RO mode).

[refack]

This https://github.com/nodejs/node/pull/12835/files#diff-745585b335912e43a534e6175f659c70 test I added to #12835 reproduces every time.

[refack]

It's like you said, we start with a regular file, remove attributes, set RO, then can't set RW.

[vsemozhetbyt]

@refack Maybe it is worth to set -a -h -i -r -s in the test to be completely sure that some weird concatenation of circumstances will not set some of them on for a new file.

[Trott]

@refack Is this still being worked on? If not, might it make sense to unassign and/or add a help wanted label?

[vsemozhetbyt]

Refs: #14926

[richardlau]

The test has been rewritten by #14926. Can this be closed or is there more work to be done?

[bzoz]

@vsemozhetbyt can this be closed now?

[vsemozhetbyt]

@richardlau , @bzoz I am not sure if rewriting the test resolves the issue in fs module.

See also #12835.

[bzoz]

This is a bug in libuv, PR to fix: libuv/libuv#1777