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

[GallaFrancesco]

After following issue 6138 on the bug tracker, 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]

Thanks for your pull request, @GallaFrancesco! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	6138	Using dirEntries and chdir() can have unwanted results

[GallaFrancesco]

I've updated the source code removing tabs.

[CyberShadow]

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

[GallaFrancesco]

@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]

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]

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]

Would this require a PR to DRuntime?

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