Fix #9584 - Using dirEntries and chdir() can have unwanted results. #6125

GallaFrancesco · 2018-02-04T19:36:38Z

After following issue 6138 on the bug tracker [edit: #9584 on GitHub], I am proposing this fix for the std.file module.

All it does is calling absolutePath from std.path before passing the argument to dirEntries.
The overload of dirEntries which also accepts string patterns doesn't seem to be affected by this, therefore no changes were proposed.

dlang-bot · 2018-02-04T19:36:40Z

Thanks for your pull request and interest in making D better, @GallaFrancesco! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#6125"

wilzbach

Thanks a lot for submitting your first PR to Phobos!
Short initial feedback:

no tabs
mind @CyberShadow's comment about potential breakage
Please add a test case

wilzbach · 2018-02-04T19:40:00Z

std/file.d

@@ -4038,7 +4038,8 @@ foreach (d; dFiles)
 +/
 auto dirEntries(string path, SpanMode mode, bool followSymlink = true)
 {
-    return DirIterator(path, mode, followSymlink);
+	import std.path: absolutePath;
+    return DirIterator(absolutePath(path), mode, followSymlink);


No tabs please (see https://dlang.org/dstyle.html)

wilzbach · 2018-02-04T19:40:09Z

std/file.d

@@ -4057,7 +4058,7 @@ auto dirEntries(string path, SpanMode mode, bool followSymlink = true)
            .array;
    }

-    void main(string[] args)
+	void main(string[] args)


GallaFrancesco · 2018-02-04T19:50:03Z

I've updated the source code removing tabs.

CyberShadow

Sorry, but as I wrote in Bugzilla, I believe that this is the wrong approach.

If you don't want to implement the more correct solution (fdopendir + openat), then I suggest that you work around this in your code.

CyberShadow · 2018-02-04T22:35:03Z

std/file.d

@@ -4109,7 +4110,7 @@ auto dirEntries(string path, SpanMode mode, bool followSymlink = true)
    foreach (string name; dirEntries(testdir, SpanMode.breadth))
    {
        //writeln(name);
-        assert(name.startsWith(testdir));
+        assert(name.startsWith(absolutePath(testdir)));


Do not break existing tests!

Changing existing unit tests to conform to changes in the implementation is almost always a mistake.

CyberShadow · 2018-02-04T22:38:12Z

@GallaFrancesco Have a look at the project tester (Jenkins) - this change actually breaks existing D code out there.

GallaFrancesco · 2018-02-04T22:42:43Z

@CyberShadow thank you for your reply.

I will look into the approach you suggested (fdopendir + openat). I wonder how this could apply to non-POSIX environments though, given that I am mainly familiar with POSIX ones. Any suggestions? Could this be solved on POSIX systems only?

CyberShadow · 2018-02-04T23:48:21Z

Windows is the only non-POSIX system we care about.

It doesn't look like Windows has native equivalents for fdopendir or openat. Here is the approach that Gnulib uses:

When beginning iteration (i.e. when dirEntries is called), save the current working directory.
Restore the working directory before every FindFileFirst call, but only for the duration of the FindFileFirst call.

However, I can't recommend the above because on Windows, the current directory is a process-wide value, so it could interfere with other threads (which are presumably aware that the user changed the current directory after calling dirEntries).

Another approach would be similar to your current implementation here, but would additionally remove the absolute path prefix in returned DirEntries, so as to not break existing code.

I think we should do the fdopendir thing on POSIX anyway, since it's more pragmatic and likely to improve performance, I'm not sure if doing the above is worthwhile on Windows... because, returning any sort of relative paths when the current directory changes is just not useful. If the path is relative to the initial working directory, then it will point to a non-existing file (or an existing file but not the one you want). If the path is relative to the current working directory, then recording it anywhere is likely to be meaningless.

Your example on Bugzilla also doesn't make sense - you're calling absolutePath twice in a row with a chdir in between, but that is not going to work right (because absolutePath uses the current directory as the base) unless the path in question was already absolute, in which case the absolutePath call is also meaningless.

So... maybe improve the POSIX implementation, because that won't hurt and could bring other benefits, but as for fixing the Bugzilla issue as it was stated... that strikes me more of a kind of "shooting yourself in the foot" situation. If you expect to change directories during directory iteration, just pass an absolute path to dirEntries.

I should add that as far as I can see, most D programs have no business at all in setting the current directory. For process creation, std.process functions can take a workDir parameter that sets directories' initial working directories. For most other things, just use std.path.buildPath & co. If your program has a "home directory" where it keeps all its work/resource files, you might be tempted to change into it when it starts, but you will later find out that this is not such a great idea when you attempt to process relative paths passed by users on your program's command line.

GallaFrancesco · 2018-02-11T18:57:41Z

I agree with you, the situations in which one needs to chdir inside a program are very few and errors such as this one or the one recently reported in the bug tracker can be solved by passing absolutePath to dirEntries.

Nevertheless, it seems to me that this behavior is poorly documented, which is the main reason I opened this PR in the first place.

The POSIX solution looks feasible though, the only issue I encountered is the absence of the fdopendir and openat declarations from DRuntime, where fdopendir is supposed to be in module dirent and openat in module fcntl.

Would this require a PR to DRuntime? I'm not sure it is worth the minimal benefits brought by these changes.

CyberShadow · 2018-02-11T22:14:32Z

Would this require a PR to DRuntime?

Improving our coverage of OS APIs is a worthwhile goal in itself.

LightBender · 2024-10-26T23:59:36Z

@thewilsonator wants to take a look at this one.

Fix Issue 6138 - Using dirEntries and chdir() can have unwanted results.

7f69b41

GallaFrancesco requested a review from CyberShadow as a code owner February 4, 2018 19:36

dlang-bot added the Severity:Bug Fix label Feb 4, 2018

wilzbach requested changes Feb 4, 2018

View reviewed changes

Fix Issue 6138 - Removed tabs according to dstyle

1dc607c

GallaFrancesco added 4 commits February 4, 2018 20:59

Fix Issue 6138 - Added test case

101f753

Fix Issue 6138 - Fixed test cleanup error

e0b8849

Fix Issue 6138 - Whitespace on import

37566b7

Fix Issue 6138 - Allman

e20e84b

CyberShadow requested changes Feb 4, 2018

View reviewed changes

CyberShadow added the Review:Needs Work label Dec 4, 2018

dlang-bot added the Merge:stalled label May 27, 2021

LightBender added the Review:Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024

0xEAB changed the title ~~Fix Issue 6138 - Using dirEntries and chdir() can have unwanted results.~~ Fix #9584 - Using dirEntries and chdir() can have unwanted results. Dec 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #9584 - Using dirEntries and chdir() can have unwanted results. #6125

Fix #9584 - Using dirEntries and chdir() can have unwanted results. #6125

GallaFrancesco commented Feb 4, 2018 •

edited by 0xEAB

Loading

dlang-bot commented Feb 4, 2018 •

edited

Loading

wilzbach left a comment

wilzbach Feb 4, 2018

wilzbach Feb 4, 2018

GallaFrancesco commented Feb 4, 2018

CyberShadow left a comment

CyberShadow Feb 4, 2018

CyberShadow commented Feb 4, 2018

GallaFrancesco commented Feb 4, 2018

CyberShadow commented Feb 4, 2018

GallaFrancesco commented Feb 11, 2018

CyberShadow commented Feb 11, 2018

LightBender commented Oct 26, 2024

Fix #9584 - Using dirEntries and chdir() can have unwanted results. #6125

Are you sure you want to change the base?

Fix #9584 - Using dirEntries and chdir() can have unwanted results. #6125

Conversation

GallaFrancesco commented Feb 4, 2018 • edited by 0xEAB Loading

dlang-bot commented Feb 4, 2018 • edited Loading

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Testing this PR locally

wilzbach left a comment

Choose a reason for hiding this comment

wilzbach Feb 4, 2018

Choose a reason for hiding this comment

wilzbach Feb 4, 2018

Choose a reason for hiding this comment

GallaFrancesco commented Feb 4, 2018

CyberShadow left a comment

Choose a reason for hiding this comment

CyberShadow Feb 4, 2018

Choose a reason for hiding this comment

CyberShadow commented Feb 4, 2018

GallaFrancesco commented Feb 4, 2018

CyberShadow commented Feb 4, 2018

GallaFrancesco commented Feb 11, 2018

CyberShadow commented Feb 11, 2018

LightBender commented Oct 26, 2024

GallaFrancesco commented Feb 4, 2018 •

edited by 0xEAB

Loading

dlang-bot commented Feb 4, 2018 •

edited

Loading