Using dirEntries and chdir() can have unwanted results

[dlangBugzillaToGithub]

andrej.mitrovich (@AndrejMitrovic) reported this on 2011-06-09T16:13:56Z

Transfered from https://issues.dlang.org/show_bug.cgi?id=6138

CC List

• andrei (@andralex)
• francesco.galla3
• greensunny12
• hsteoh
• razvan.nitu1305
• timothee.cour2

Description

I can't officially classify this as a bug. Jonathan said it might be a good idea to put it here just so it doesn't get lost, and then we can later decide if it's a bug or if we should just be careful what we do. 

Here's some buggy code which might throw an exception (if you're lucky!):
   foreach (string entry; dirEntries(curdir, SpanMode.shallow))
   {
       if (entry.isdir)
       {
           foreach (string subentry; dirEntries(entry, SpanMode.shallow))
           {
               if (subentry.isdir)
               {
                   chdir(rel2abs(subentry));
               }
           }
       }
   }

This might throw something like this:
std.file.FileException@std\file.d(1124): .\foo\bar.d: The system cannot find the path specified.

The problem is chdir will modify curdir, and dirEntries doesn't store 'curdir' internally as an absolute address, so calling chdir() screws up the behavior of dirEntries.

A workaround is to call rel2abs as soon as possible when using 'curdir' (the first line changed here):

   foreach (string entry; dirEntries(rel2abs(curdir), SpanMode.shallow))
   {
       if (entry.isdir)
       {
           foreach (string subentry; dirEntries(entry, SpanMode.shallow))
           {
               if (subentry.isdir)
               {
                   chdir(rel2abs(subentry));
               }
           }
       }
   }

I've had a use for code like this where I was invoking a script file which was always relative to some subdirectory.

[dlangBugzillaToGithub]

timothee.cour2 commented on 2016-03-23T05:33:36Z

sounds like a bug (and one that could cause harm); 
why not have dirEntries call rel2abs ? the cost of this should be dwarfed by system calls involved in dirEntries

[dlangBugzillaToGithub]

razvan.nitu1305 commented on 2017-07-07T12:53:53Z

Is this still valid? The code seems to have changes significantly (rel2abs doesn't exist anymore) and I cannot reproduce with std.file.absolutePath.

[dlangBugzillaToGithub]

razvan.nitu1305 commented on 2017-07-07T13:00:50Z

After analyzing the code I see that dirEntries does not call absolutePath on the path, which in my opinion is a bug. Even though I cannot reproduce the bug, I think a call to absolutePath should definitely be added

[dlangBugzillaToGithub]

dlang-bugzilla (@CyberShadow) commented on 2017-07-07T14:52:56Z

(In reply to RazvanN from comment #3)
> After analyzing the code I see that dirEntries does not call absolutePath on
> the path, which in my opinion is a bug. Even though I cannot reproduce the
> bug, I think a call to absolutePath should definitely be added

That might break things.

~ $ mkdir -p a/b/c
~ $ cd a/b/c
~/a/b/c $ touch a
~/a/b/c $ ls
a
~/a/b/c $ ls ~/a/b/c
a
~/a/b/c $ chmod 000 ~/a/b
~/a/b/c $ ls
a
~/a/b/c $ ls ~/a/b/c
ls: cannot access '/home/vladimir/a/b/c': Permission denied
~/a/b/c $ 

I think the correct solution would be to use fdopendir and openat (on POSIX).

[dlangBugzillaToGithub]

francesco.galla3 commented on 2018-01-29T13:14:54Z

(In reply to RazvanN from comment #3)
> After analyzing the code I see that dirEntries does not call absolutePath on
> the path, which in my opinion is a bug. Even though I cannot reproduce the
> bug, I think a call to absolutePath should definitely be added

I reproduced the bug with the following code:

   foreach (string entry; dirEntries((dir), SpanMode.shallow))
   {
       if (entry.isDir)
       {
	    foreach (string subentry; dirEntries(entry, SpanMode.shallow))
            {
                 if (subentry.isDir)
		 {
                     chdir(absolutePath(subentry));
		     writeln (absolutePath(subentry));
		 }
	    }
       }
   }

My directory tree is:

.
├── 1
│   └── 2
├── 3
│   ├── 4
│   └── 5
│       └── 6


* The result I obtained was the following:

std.file.FileException@/opt/dmd-2.075/import/std/file.d(1631): ./3: No such file or directory

* By calling:

foreach (string entry; dirEntries(absolutePath(dir), SpanMode.shallow))

The code was executed correctly. This seems to confirm the need for dirEntries() to call absolutePath(). Am I mistaken?

[dlangBugzillaToGithub]

greensunny12 commented on 2018-02-04T19:43:16Z

FYI: Francesco Galla submitted a PR - https://github.com/dlang/phobos/pull/6125

[dlangBugzillaToGithub]

timothee.cour2 commented on 2018-02-10T03:25:20Z

just hit that bug again, but differently:
```
chdir(foo);
auto files=dirEntries(dirSrc, "*.d", SpanMode.depth).filter!(a=>a.isFile).map!(a=>dir.buildPath(a.name)).array;
```
crashed similarly

[dlangBugzillaToGithub]

timothee.cour2 commented on 2018-03-16T22:16:18Z

ping: this is a serious bug
note: i was OSX and the bug was reported on windows originally, so setting to All

[0xEAB]

This issue – at least the actually reported one – is still reproducible.

One might not agree on the premise, but given that this was reported by multiple people over the course of years, we should probably do something about it:

DirEntry implicitly converting to a potentially relative path thanks to its alias this has proven to be a footgun IMHO.

[0xEAB]

@CyberShadow commented on 2017-07-07T14:52:56Z

That might break things.

As far as I can tell, this issue isn’t a thing on Windows (with NTFS at least).

C:\>cd C:\test
Access is denied.

C:\>cd C:\test\nested

C:\test\nested>notepad file.txt

C:\test\nested>type file.txt
asdf

Current user has no permissions on C:\test, a folder that is owned by a different users. While C:\test\nested is also owned by a different user, the current one does have permissions AND can access it in practice.