974677 - OS.File should not follow symlink when it removes a directory

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

Steps to reproduce:

1. Symlink an invalid add-on to your extensions folder
2. run firefox

Expected:

the symlink is removed

Actual:

the symlinked folder is removed

Comment 1

[Masatoshi Kimura [:emk]]

OS.File.removeDir has a defect.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

To fix this, it should be sufficient to change |AbstractFile.removeDir| (in osfile_shared_front.jsm) to recursively call |removeDir| only if |entry.isDir && !entry.isSymLink|.

To test this, we would need to also make function |symlink| (available in osfile_unix_back.jsm) public as |OS.File.unixSymlink| (in osfile_unix_front.jsm).

Marking as mentored bug. emk, by any chance, do you want to handle this?

Comment 3

[Masatoshi Kimura [:emk]]

We will have to consider |path| itself may be a symlink to a directory.

Comment 4

[Masatoshi Kimura [:emk]]

And yes, I'll look into this.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Stop following symlinks when OS.File removes a directory

- I separated the removeDir function because to avoid lstat or
  GetFileAttributes being called for each file entry under the
  specified directory.
- I didn't add OS.File.unixSymlink because OS.Unix.File.symlink was
  already visible from the test.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=d1cdaa057f64

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8379712 [details] [diff] [review]
Stop following symlinks when OS.File removes a directory

Review of attachment 8379712 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +470,5 @@
>    *
>    * @throws {OS.File.Error} In case of I/O error, in particular if |path| is
>              not a directory.
>    */
> +AbstractFile.removeRecursive = function(path, options = {}) {

So what's the difference between removeRecursive and removeDir?

@@ +483,5 @@
>      for (let entry in iterator) {
>        if (entry.isDir) {
> +        if (entry.isLink) {
> +          // NTFS junctions or NTFS symlinks to directories.
> +          OS.File.removeEmptyDir(entry.path, options);

Why would removeEmptyDir work here? Shouldn't this be OS.File.remove(entry.path)?

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +923,5 @@
>       File.writeAtomic = exports.OS.Shared.AbstractFile.writeAtomic;
>       File.openUnique = exports.OS.Shared.AbstractFile.openUnique;
> +
> +     /**
> +      * Remove a directory with consideration of symbolic links.

Since this is the public API, please move the existing documentation here.

@@ +944,5 @@
> +         throw e;
> +       }
> +       if (isSymLink) {
> +         if (!isDir) {
> +           throw new File.Error("removeDir", Const.ENOTDIR);

For compatibility with bug 967507, can you add as third argument |path|?

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +973,5 @@
> +         if ((!("ignoreAbsent" in options) || options.ignoreAbsent) &&
> +             ctypes.winLastError == Const.ERROR_FILE_NOT_FOUND) {
> +           return;
> +         }
> +         throw new File.Error("removeEmptyDir");

For compatibility with bug 967507, can you add arguments |ctypes.winLastError| and |path|?

@@ +976,5 @@
> +         }
> +         throw new File.Error("removeEmptyDir");
> +       }
> +       if (attributes & Const.FILE_ATTRIBUTE_REPARSE_POINT) {
> +         OS.File.removeEmptyDir(path, options);

Here too, I don't understand why removeEmptyDir would work here.

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js
@@ +14,5 @@
>    test_create_file();
>    test_access();
>    test_read_write();
>    test_passing_undefined();
> +  test_symlink();

Please make this rather a xpcshell test. We're trying to get rid of the mochitests here.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Stop following symlinks when OS.File removes a directory, v2

Also fixed a bug of File.stat. The added test caught the bug :)

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #6)
> > +AbstractFile.removeRecursive = function(path, options = {}) {
> 
> So what's the difference between removeRecursive and removeDir?

removeRecursive doesn't check if the path itself is a link. I separated the function to avoid calling lstat for each files under the directory. If you think the performance impact is negligible, I'll merge the functions.

> > +        if (entry.isLink) {
> > +          // NTFS junctions or NTFS symlinks to directories.
> > +          OS.File.removeEmptyDir(entry.path, options);
> 
> Why would removeEmptyDir work here? Shouldn't this be
> OS.File.remove(entry.path)?

Because NTFS symlinks to directories and junctions are directories themselves unlike Unix symlinks. Rather, OS.File.remove() will not work here.

> > +     /**
> > +      * Remove a directory with consideration of symbolic links.
> 
> Since this is the public API, please move the existing documentation here.

Done.

> > +           throw new File.Error("removeDir", Const.ENOTDIR);
> 
> For compatibility with bug 967507, can you add as third argument |path|?

OK.

> > +         throw new File.Error("removeEmptyDir");
> 
> For compatibility with bug 967507, can you add arguments
> |ctypes.winLastError| and |path|?

Done.

> > +       if (attributes & Const.FILE_ATTRIBUTE_REPARSE_POINT) {
> > +         OS.File.removeEmptyDir(path, options);
> 
> Here too, I don't understand why removeEmptyDir would work here.

See above. On Windows, removeEmptyDir is implemented by RemoveDirectory which will not follow reparse points.

> > +  test_symlink();
> 
> Please make this rather a xpcshell test. We're trying to get rid of the
> mochitests here.

Done.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: interdiff

For ease of review.

Comment 9

[Masatoshi Kimura [:emk]]

Oh, and I had to add OS.File.unixSymLink after all because the test no longer runs on a worker thread.

Comment 10

[Masatoshi Kimura [:emk]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=faafe8544508

Comment 11

[David Teller [:Yoric] - still alive but not very active]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> removeRecursive doesn't check if the path itself is a link. I separated the
> function to avoid calling lstat for each files under the directory. If you
> think the performance impact is negligible, I'll merge the functions.

Saving the lstat is good. Please make this clearer in the comments.

> 
> > > +        if (entry.isLink) {
> > > +          // NTFS junctions or NTFS symlinks to directories.
> > > +          OS.File.removeEmptyDir(entry.path, options);
> > 
> > Why would removeEmptyDir work here? Shouldn't this be
> > OS.File.remove(entry.path)?
> 
> Because NTFS symlinks to directories and junctions are directories
> themselves unlike Unix symlinks. Rather, OS.File.remove() will not work here.

Ok, please make this clear in the comments.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8383709 [details] [diff] [review]
Stop following symlinks when OS.File removes a directory, v2

Review of attachment 8383709 [details] [diff] [review]:
-----------------------------------------------------------------

Good for me, with a few minor changes.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +845,5 @@
> + * @returns {Promise}
> + * @rejects {OS.File.Error} In case of any error.
> + *
> + * General note: Currently this function is implemented only on
> + * Unix-like platforms.

That last line is not useful.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +967,5 @@
> +         throw e;
> +       }
> +       if (isSymLink) {
> +         if (!isDir) {
> +           throw new File.Error("removeDir", Const.ENOTDIR, path);

I don't understand that test. Did you mean to put it outside of the |if (isSymLink)| test?

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +998,5 @@
> +         }
> +         throw new File.Error("removeEmptyDir", ctypes.winLastError, path);
> +       }
> +       if (attributes & Const.FILE_ATTRIBUTE_REPARSE_POINT) {
> +         OS.File.removeEmptyDir(path, options);

Please add a comment here explaining why removeEmptyDir is the right tool for that task.

@@ +1000,5 @@
> +       }
> +       if (attributes & Const.FILE_ATTRIBUTE_REPARSE_POINT) {
> +         OS.File.removeEmptyDir(path, options);
> +         return;
> +       }

So here you're not checking whether |path| points to a directory, are you? That's probably not going to change anything (e.g. opening the DirectoryIterator will fail), but that's a bit surprising.

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js
@@ +196,5 @@
>      exceptionRaised = true;
>    }
>  
>    ok(exceptionRaised, "test_passing_undefined: exception gets thrown")
>  }

Can you revert this whitespace change?

::: toolkit/components/osfile/tests/xpcshell/test_removeDir.js
@@ +84,5 @@
>    yield OS.File.makeDir(subDir);
>    yield OS.File.writeAtomic(fileInSubDir, "content", { tmpPath: fileInSubDir + ".tmp" });
>    yield OS.File.removeDir(dir);
>    do_check_false((yield OS.File.exists(dir)));
> +

Can you move this test to a different task?
If people add tests at the end of this task, it would be a shame if they weren't executed under Windows.

@@ +91,5 @@
> +    return;
> +  }
> +
> +  // The preceding test should have created |file|.
> +  do_check_true((yield OS.File.exists(file)), file + " exists");

There is no second argument to do_check_* (which is a shame).
You'll need to use do_print() to print something on the output. Just explain what you're doing, no need to document every single do_check_*.

@@ +105,5 @@
> +  info = yield OS.File.stat(file + ".link");
> +  do_check_false(info.isDir, file + ".link target is not a directory");
> +  do_check_false(info.isSymLink, file + ".link target is not a symlink");
> +  yield OS.File.remove(file + ".link");
> +  do_check_false((yield OS.File.exists(file + ".link")), file + ".link is removed");

The symlink tests should go to another file, perhaps in test_symlink.js.

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Stop following symlinks when OS.File removes a directory, v3

(In reply to David Rajchenbach Teller [:Yoric] (currently away from bugzilla – ETA March 11th – please use "needinfo?") from comment #12)
> ::: toolkit/components/osfile/modules/osfile_unix_front.jsm
> @@ +967,5 @@
> > +         throw e;
> > +       }
> > +       if (isSymLink) {
> > +         if (!isDir) {
> > +           throw new File.Error("removeDir", Const.ENOTDIR, path);
> 
> I don't understand that test. Did you mean to put it outside of the |if
> (isSymLink)| test?

If the test is not present, removeDir() can remove symlinks to directories on Unix, but cannot on Windows. I added the test for consistency between platforms. If you think it does not matter so much here, I'll remove the check.
I added the explicit check into the |if (isSymLink)| block because of two reasons:
1. For regular files, removeEmptyDir() will fail anyway. We can save a syscall by not checking explicitly.
2. The operating system will check and remove the path if it is a directory atomically. It is impossible for us.

> ::: toolkit/components/osfile/modules/osfile_win_front.jsm
> @@ +1000,5 @@
> > +       }
> > +       if (attributes & Const.FILE_ATTRIBUTE_REPARSE_POINT) {
> > +         OS.File.removeEmptyDir(path, options);
> > +         return;
> > +       }
> 
> So here you're not checking whether |path| points to a directory, are you?
> That's probably not going to change anything (e.g. opening the
> DirectoryIterator will fail), but that's a bit surprising.

I didn't add an explicit check for the same reason as Unix (see above).

> ::: toolkit/components/osfile/tests/xpcshell/test_removeDir.js
> @@ +84,5 @@
> >    yield OS.File.makeDir(subDir);
> >    yield OS.File.writeAtomic(fileInSubDir, "content", { tmpPath: fileInSubDir + ".tmp" });
> >    yield OS.File.removeDir(dir);
> >    do_check_false((yield OS.File.exists(dir)));
> > +
> 
> Can you move this test to a different task?

Moved to a different file (test_symlink.js).

> If people add tests at the end of this task, it would be a shame if they
> weren't executed under Windows.

Added a comment at the bottom of the new file.

Other points are resolved.

https://tbpl.mozilla.org/?tree=Try&rev=a34634351c13

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #13)
> If the test is not present, removeDir() can remove symlinks to directories
> on Unix, but cannot on Windows.

Correction: removeDir() can remove symlinks to *files* on Unix, but cannot on Windows.
Sorry for the confusion.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

(In reply to Masatoshi Kimura [:emk] from comment #13)
> > > +       if (isSymLink) {
> > > +         if (!isDir) {
> > > +           throw new File.Error("removeDir", Const.ENOTDIR, path);
> > 
> > I don't understand that test. Did you mean to put it outside of the |if
> > (isSymLink)| test?
> 
> If the test is not present, removeDir() can remove symlinks to directories
> on Unix, but cannot on Windows. I added the test for consistency between
> platforms. If you think it does not matter so much here, I'll remove the
> check.
> I added the explicit check into the |if (isSymLink)| block because of two
> reasons:
> 1. For regular files, removeEmptyDir() will fail anyway. We can save a
> syscall by not checking explicitly.
> 2. The operating system will check and remove the path if it is a directory
> atomically. It is impossible for us.

I haven't double-checked, but according to your source comment, « A Unix symlink is not a directory even if it points to a directory. » Here, since you have already checked whether it's a symlink, how can it also be a directory?

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8389463 [details] [diff] [review]
Stop following symlinks when OS.File removes a directory, v3

Review of attachment 8389463 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling review until I understand the !isDir + isSymlink test.

::: toolkit/components/osfile/tests/xpcshell/test_removeDir.js
@@ +89,5 @@
> +  // The preceding test should have created |file|.
> +  do_check_true((yield OS.File.exists(file)), file + " exists");
> +  let info = yield OS.File.stat(file, {unixNoFollowingLinks: true});
> +  do_check_false(info.isDir, file + " is not a directory");
> +  do_check_false(info.isSymLink, file + " is not a symlink");

What's the point of these checks?

::: toolkit/components/osfile/tests/xpcshell/test_symlink.js
@@ +9,5 @@
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +do_register_cleanup(function() {
> +  Services.prefs.setBoolPref("toolkit.osfile.log", false);
> +});

You don't need to tweak that pref, it's set automatically for all xpcshell tests in this directory.

@@ +17,5 @@
> +
> +  run_next_test();
> +}
> +
> +add_task(function() {

function*, please,
but really, since what you are testing is actually removeDir, I believe that this should go back into the previous file – just in a different task.

@@ +20,5 @@
> +
> +add_task(function() {
> +  // Set up profile. We create the directory in the profile, because the profile
> +  // is removed after every test run.
> +  do_get_profile();

It's probably nicer to create the profile in run_test(), since it's part of initialization.

@@ +32,5 @@
> +
> +  // Windows does not implement OS.File.unixSymLink()
> +  if (OS.Constants.Win) {
> +    return;
> +  }

Bail out at the start of the task, for the sake of clarity (and performance).

@@ +92,5 @@
> +  do_check_false((yield OS.File.exists(dir3)));
> +
> +  // This task will be executed only on Unix-like systems.
> +  // Please do not add tests independent to operating systems here
> +  // or implement symlink() on Windows.

To make this clearer, just call the task
function* test_unix_symlink()

Comment 17

[Masatoshi Kimura [:emk]]

(In reply to David Rajchenbach Teller [:Yoric] (currently away from bugzilla – ETA March 11th – please use "needinfo?") from comment #15)
> I haven't double-checked, but according to your source comment, « A Unix
> symlink is not a directory even if it points to a directory. » Here, since
> you have already checked whether it's a symlink, how can it also be a
> directory?

See the above code carefully. UnixFile.removeDir() will first check the path parameter using OS.File.stat() with {unixNoFollowingLinks: true}. If the path is a symlink, then the function will check the link target using OS.File.stat *without* {unixNoFollowingLinks: true}.
So this functions will behave as follows:
1. If the path is a symlink and the target is not a directory, throws an exception.
2. If the path is a symlink and the target is a directory, call OS.File.remove() to remove the symlink.
3. If the path is not a symlink, call removeRecursive() to remove the directory.
Note that 1. and 2. will not check if the link itself is a directory.

Comment 18

[Masatoshi Kimura [:emk]]

(In reply to David Rajchenbach Teller [:Yoric] (currently away from bugzilla – ETA March 11th – please use "needinfo?") from comment #16)
> ::: toolkit/components/osfile/tests/xpcshell/test_symlink.js
> @@ +17,5 @@
> > +
> > +  run_next_test();
> > +}
> > +
> > +add_task(function() {
> 
> function*, please,
> but really, since what you are testing is actually removeDir, I believe that
> this should go back into the previous file – just in a different task.

It's the very reason why I put the test in that file first.
The purpose of this test is to make sure that removeDir() will work correctly when the path contains symlinks or when the path itself is a symlink. So this test *requires* to cretate symlinks, so it is impossible to separate the removeDir() calls from the unixSymLink() calls.
If removeDir() calls should be moved back to test_removeDir.js, unixSymLink() calls must be moved back either, so I can't create test_symlink.js (or test_unixsymlink.js, or whatever names you prefer). If unixSymLink() calls should be moved to a new file, removeDir() must be moved too. Which do you prefer?

Comment 19

[David Teller [:Yoric] - still alive but not very active]

(In reply to Masatoshi Kimura [:emk] from comment #17)
> (In reply to David Rajchenbach Teller [:Yoric] (currently away from bugzilla
> – ETA March 11th – please use "needinfo?") from comment #15)
> > I haven't double-checked, but according to your source comment, « A Unix
> > symlink is not a directory even if it points to a directory. » Here, since
> > you have already checked whether it's a symlink, how can it also be a
> > directory?
> 
> See the above code carefully. 

Ok, I finally understood what I was missing.
I believe that we can safely remove all the |isDir| check. It might be interesting to add a note somewhere in the documentation explaining that, if |path| is a symlink, we don't check whether it points to a directory.

> It's the very reason why I put the test in that file first.

Ok, I had mistakenly understood that lines 38-70 were actually testing unixSymLink. Fair enough, let's put it back where it was in the first place.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: Stop following symlinks when OS.File removes a directory, v4

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #16)
> @@ +17,5 @@
> > +
> > +  run_next_test();
> > +}
> > +
> > +add_task(function() {
> 
> function*, please,

Done.

> @@ +32,5 @@
> > +
> > +  // Windows does not implement OS.File.unixSymLink()
> > +  if (OS.Constants.Win) {
> > +    return;
> > +  }
> 
> Bail out at the start of the task, for the sake of clarity (and performance).

Done.

> @@ +92,5 @@
> > +  do_check_false((yield OS.File.exists(dir3)));
> > +
> > +  // This task will be executed only on Unix-like systems.
> > +  // Please do not add tests independent to operating systems here
> > +  // or implement symlink() on Windows.
> 
> To make this clearer, just call the task
> function* test_unix_symlink()

Done.

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #19)
> Ok, I finally understood what I was missing.
> I believe that we can safely remove all the |isDir| check. It might be
> interesting to add a note somewhere in the documentation explaining that, if
> |path| is a symlink, we don't check whether it points to a directory.

Removed the |isDir| checks and added a note to the method comment.

> Ok, I had mistakenly understood that lines 38-70 were actually testing
> unixSymLink. Fair enough, let's put it back where it was in the first place.

Moved back to test_removeDir.js.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0cfafd3d367c

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8390881 [details] [diff] [review]
Stop following symlinks when OS.File removes a directory, v4

Review of attachment 8390881 [details] [diff] [review]:
-----------------------------------------------------------------

Good for me, with a few minor changes.
If you wish, you can ask for another review once you have made these changes, your call.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +849,5 @@
> + *
> + * @returns {Promise}
> + * @rejects {OS.File.Error} In case of any error.
> + */
> +File.unixSymLink = function unixSymLink(sourcePath, destPath) {

We should define this function only under Unix. You can detect whether we're under Unix by checking whether SharedAll.Constants.Win is undefined.

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +323,5 @@
>     move: function move(sourcePath, destPath, options) {
>       return File.move(Type.path.fromMsg(sourcePath),
>         Type.path.fromMsg(destPath), options);
>     },
> +   unixSymLink: function unixSymLink(sourcePath, destPath) {

This should probably also be defined only under Unix.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +988,5 @@
> +      * @throws {OS.File.Error} In case of I/O error, in particular if |path| is
> +      *         not a directory.
> +      */
> +     File.removeDir = function(path, options) {
> +       // We can't use File.stat here because it will follow the symlink.

Can you file a followup bug for introducing the equivalent of unixNoFollowSymLinks for Windows File.stat()?

::: toolkit/components/osfile/tests/xpcshell/test_removeDir.js
@@ +98,5 @@
> +  let file1 = OS.Path.join(dir, "file1");
> +
> +  // Sanity checking for the test
> +  do_check_false((yield OS.File.exists(dir)));
> +

Can you add a small comment summarizing the layout of the directory you are creating?

Comment 22

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/550f66e6106e

Comment 23

[Masatoshi Kimura [:emk]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #21)
> Can you file a followup bug for introducing the equivalent of
> unixNoFollowSymLinks for Windows File.stat()?

Filed bug 986315.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/550f66e6106e