fs: refactor lchmod to not rely on write permission

Refactor to remove the use of O_WRONLY for lchmod, lchmodSync and promisified lchmod. Fixes: nodejs#23736
Sayanc93 · Oct 26, 2018 · 4accca8 · 4accca8
1 parent c829202
commit 4accca8
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 5 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -35,7 +35,7 @@ const {
   R_OK,
   W_OK,
   X_OK,
-  O_WRONLY,
+  O_RDONLY,
   O_SYMLINK
 } = constants;
 
@@ -987,7 +987,7 @@ function fchmodSync(fd, mode) {
 
 function lchmod(path, mode, callback) {
   callback = maybeCallback(callback);
-  fs.open(path, O_WRONLY | O_SYMLINK, function(err, fd) {
+  fs.open(path, O_RDONLY | O_SYMLINK, function(err, fd) {
     if (err) {
       callback(err);
       return;
@@ -1003,7 +1003,7 @@ function lchmod(path, mode, callback) {
 }
 
 function lchmodSync(path, mode) {
-  const fd = fs.openSync(path, O_WRONLY | O_SYMLINK);
+  const fd = fs.openSync(path, O_RDONLY | O_SYMLINK);
 
   // Prefer to return the chmod error, if one occurs,
   // but still try to close, and report closing errors if they occur.

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -3,7 +3,7 @@
 const {
   F_OK,
   O_SYMLINK,
-  O_WRONLY,
+  O_RDONLY,
   S_IFMT,
   S_IFREG
 } = internalBinding('constants').fs;
@@ -400,7 +400,7 @@ async function lchmod(path, mode) {
   if (O_SYMLINK === undefined)
     throw new ERR_METHOD_NOT_IMPLEMENTED('lchmod()');
 
-  const fd = await open(path, O_WRONLY | O_SYMLINK);
+  const fd = await open(path, O_RDONLY | O_SYMLINK);
   return fchmod(fd, mode).finally(fd.close.bind(fd));
 }
 

diff --git a/test/parallel/test-fs-chmod-mask.js b/test/parallel/test-fs-chmod-mask.js
@@ -16,6 +16,7 @@ if (common.isWindows) {
 }
 
 const maskToIgnore = 0o10000;
+const kMaskOnlyRead = 0o444;
 
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
@@ -89,6 +90,34 @@ function test(mode, asString) {
     fs.lchmodSync(link, input);
     assert.strictEqual(fs.lstatSync(link).mode & 0o777, mode);
   }
+
+  // Test that `lchmod` works on readonly (0x444) files.
+  if (fs.lchmod) {
+    const link = path.join(tmpdir.path, `lchmod-read-only-src-${suffix}`);
+    const file = path.join(tmpdir.path, `lchmod-read-only-dest-${suffix}`);
+    fs.writeFileSync(file, 'test', { encoding: 'utf-8', mode: 0o444 });
+    fs.symlinkSync(file, link);
+
+    fs.lchmod(link, input, common.mustCall((err) => {
+      assert.ifError(err);
+      assert.strictEqual(
+        fs.lstatSync(link).mode & kMaskOnlyRead, kMaskOnlyRead
+      );
+    }));
+  }
+
+  // Test that `lchmodSync` works on readonly (0x444) files.
+  if (fs.lchmodSync) {
+    const link = path.join(tmpdir.path, `lchmodSync-read-only-src-${suffix}`);
+    const file = path.join(tmpdir.path, `lchmodSync-read-only-dest-${suffix}`);
+    fs.writeFileSync(file, 'test', { encoding: 'utf-8', mode: 0o444 });
+    fs.symlinkSync(file, link);
+
+    for (const _mode of [...Array(1..mode).keys()]) {
+      fs.lchmodSync(link, _mode + 1);
+      assert.strictEqual(fs.lstatSync(link).mode & 0o777, _mode + 1);
+    }
+  }
 }
 
 test(mode, true);