nsIFile symlink attribute inconsistencies.

NEW
Unassigned

Status

()

Core
XPCOM
15 years ago
2 years ago

People

(Reporter: dougt, Unassigned)

Tracking

(Blocks: 2 bugs, {helpwanted})

Trunk
Future
x86
Windows XP
helpwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

15 years ago
This bug is fallout from bug 201381.

Consider a nsIFile abstracting the location 

/a/<symlink>/directory

Where /a would be c:/ on windows.

On unix:
isDirectory True
isSymlink   False

on Windows:
isDirectory: True
isSymlink:   True

Windows is reporting this file as symlink (read shortcut) since a symlink is in
the path.  We need to either fix this to be consistent or document the
inconsistencies.
(Reporter)

Comment 1

15 years ago
Window's impl. of IsSymlink() just checks the leaf; unix lstat's the path and
returns S_ISLNK.  what was I thinking?  

(Reporter)

Comment 2

15 years ago
IsDirectory returns true if a terminal symlink resolves to a directory.  
(Reporter)

Comment 3

15 years ago
I need your comments, please.  Should the terminal node be resolved when a
client asks for answers to:

isDirectory, isFile, isReadable, isExecutable, isHidden, isSymlink, exists?

When answering these questions, should we resolve the terminal symlink.  We
currently have three *OfLink methods which imply the default behavior is to
resolve the symlink.  We also have Open* and Load methods of the nsILocalFile
interface which work on the resolved terminal node.

A directory is not a symlink.  A symlink to a directory is not a directory.  Let
the caller follow the link (call stat instead of lstat) if it needs the target
file type.  Or so the old Unix fart in me says.

Since we currently lack XP semantics here, what does a quick lxr scan say about
callers' expectations?  Are there bugs on some platforms but not on others due
to the callers expecting lstat instead of stat on Windows, and vice versa on Unix?

/be
(Reporter)

Comment 5

15 years ago
>A directory is not a symlink.  A symlink to a directory is not a directory.  Let
the caller follow the link (call stat instead of lstat) if it needs the target
file type.  Or so the old Unix fart in me says.

We alreadly autoresolve the terminal in other predicates and methods.  For
example, OpenNSPRFile will resolve the symlink to a destination file and open
that.  Getting the last modification date of a symlink will return the date of
the resolve file.  Are you suggesting that we handle a terminal symlink that
points to a directory differently?

If this is any kind of measure of usage, there are only about 4 callers of
IsSymlink.
> Are you suggesting that we handle a terminal symlink that points to a directory
> differently?

No, I'm very explicitly saying that *file type testing* (the isDirectory,
isSymlink, etc. methods of nsIFile) should not follow terminal symlinks.

That's what Unix lstat does.  There is no "lopen" -- one uses readlink to read a
symlink atomicly -- and open follows terminal symlinks, as do most system calls.
Symlinks should be transparent, except when you need to know they're there. E.g.
when enumerating IE bookmark folders, and you want to skip symlinks to
directories outside the hierarchy.

If we don't have a type-testing model that distinguishes symlinks from
directories, we can't handle such cases.  If we do have a model (and we seem to
now, but it's inconsistent), then it should be consistent and minimal (no true
results for two different methods on the same nsIFile).

/be
(Reporter)

Comment 7

15 years ago
sure, i can buy that.  Suppose that we do this "file type testing" only.  

what does GetLastModifiedTime do?
what does GetLastModifiedTimeOfLink do?

If we say GetLastModifiedTime resolves the terminal (since we have the _OfLink
method), this still has an inconsistency.
I don't see an inconsistency: lastModificationTime, like most operations (like
most Unix system calls taking pathname parameters), to make symlinks transparent
by default, should follow links.

But file typing needs to be consistent in this sense a file has one type.  It
also needs to disclose that the type of a symlink (a pathname whose terminal
component names a symbolic link) is, indeed, "symlink" (else bookmarks and other
use-cases can't do the right thing).

You might have isDirectory and isDirectoryOfLink, but that's bad English, and it
makes for way too many methods.  If you exposed a file type enum, then you might
do better to have fileType and fileTypeOfLink, but nsIFile is FROZEN.  So the
right solution is to make the implementations consistent in the only way that
preserves type uniqueness and allows links to be discovered.

/be
(Reporter)

Comment 9

15 years ago
Created attachment 125327 [details] [diff] [review]
Patch v.1
Comment on attachment 125327 [details] [diff] [review]
Patch v.1

(Doug, can you add diff -pu8 or something with even more context to your
.cvsrc? Thanks.)
(Reporter)

Comment 11

15 years ago
I tried the above patch and we no longer hang importing the c:/ IE bookmark --
we do not import this bookmark at all.  I am using version 1.290 of
nsBookmarksService.cpp
(Reporter)

Comment 12

15 years ago
Created attachment 125331 [details] [diff] [review]
patch v.2
Attachment #125327 - Attachment is obsolete: true
Comment on attachment 125331 [details] [diff] [review]
patch v.2

>+++ nsLocalFileWin.cpp	10 Jun 2003 19:11:50 -0000
>@@ -1747,33 +1747,33 @@ nsLocalFile::GetParent(nsIFile * *aParen
> }
> 
> NS_IMETHODIMP  
> nsLocalFile::Exists(PRBool *_retval)
> {
>     NS_ENSURE_ARG(_retval);
> 
>     MakeDirty();
>-    nsresult rv = ResolveAndStat( PR_TRUE );
>+    nsresult rv = ResolveAndStat( PR_FALSE );

Hey, it's 1.5alpha: why not clean up the inconsistent overly-spacey parens to
match the norm (no, spaces, except, after, commas)?

>     

Lots of trailing whitespace, a few bogus tabs too.  Nab' em?

>     if (NS_SUCCEEDED(rv))
>         *_retval = PR_TRUE;
>     else 
>         *_retval = PR_FALSE;

This could be just *_retval = NS_SUCCEEDED(rv); -- you can count on
NS_SUCCEEDED resulting in a boolean (0 or 1) value.

> nsLocalFile::IsSymlink(PRBool *_retval)
> {
>     NS_ENSURE_ARG(_retval);
>     *_retval = PR_FALSE;
> 
>-    nsCAutoString path;
>-    int   pathLen;
>-    
>-    GetNativePath(path);
>-    pathLen = path.Length();
>-    
>-    const char* leaf = path.get() + pathLen - 4;
>-    
>+    const char* leaf = mWorkingPath.get() + mWorkingPath.Length() - 4;
>+ 
>     if ( (strcmp(leaf, ".lnk") == 0)) 

What if mWorkingPath.Length() is < 4?

> nsLocalFile::IsSpecial(PRBool *_retval)
> {
>     NS_ENSURE_ARG(_retval);
>     *_retval = PR_FALSE;
> 
>-    nsresult rv = ResolveAndStat(PR_TRUE);
>+    nsresult rv = ResolveAndStat(PR_FALSE);
>     
>     if (NS_FAILED(rv))
>         return rv;
>-    
>+     

Even if you don't pick the trailing space nits, don't add any more.

/be
Bringing ben in so he can help figure out what the bookmarks code should do in
the case of bug 201381, given that a Windows nsIFile implementation fixed as
proposed here won't report a shortcut to a directory as being isDirectory(), but
will answer true from isSymlink().

/be
(Reporter)

Comment 15

15 years ago
Created attachment 125344 [details] [diff] [review]
patch v.3

fixes bug and nits
Attachment #125331 - Attachment is obsolete: true
(Reporter)

Comment 16

15 years ago
Created attachment 125424 [details] [diff] [review]
patch v.4

contains clarification comments in nsIFile.idl
Attachment #125344 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #125424 - Flags: superreview?(brendan)
Attachment #125424 - Flags: review?(darin)

Comment 17

15 years ago
Comment on attachment 125424 [details] [diff] [review]
patch v.4

>Index: nsIFile.idl

>+    /**
>+     * File type tests.  These methods return information about 
>+     * the unresolved leaf. 
>+     */
>     boolean exists();
>     boolean isWritable();
>     boolean isReadable();
>     boolean isExecutable();
>     boolean isHidden();
>     boolean isDirectory();
>     boolean isFile();
>     boolean isSymlink();

hmm... unless the nsIFile was created with followLinks set to true??
what is the behavior of that parameter anyways?  it seems to be another
thing that is not consistent XP.


>Index: nsLocalFileWin.cpp

>+    if (status == PR_SUCCESS)
> 		mDirty = PR_FALSE;
>     else
>         result = NS_ERROR_FILE_NOT_FOUND;
> 
> 	return result;

kill old tabs =)


>+    if (NS_SUCCEEDED(rv) && localFile)
>     {
>         return CallQueryInterface(localFile, aParent);
>     }
>     return rv;
> }

slight codesize optimization (while you're in the neighborhood):

      if (NS_SUCCEEDED(rv) && localFile)
      {
	  rv = CallQueryInterface(localFile, aParent);
      }
      return rv;


>+    rc = desktopFolder->BindToObject(folder_pidl,
>                                       0,
>                                       IID_IShellFolder,
>-                                      (void**)&folder );
>+                                      (void**)&folder);

nit: align parameters while you're at it
(Reporter)

Comment 18

15 years ago
>hmm... unless the nsIFile was created with followLinks set to true??
what is the behavior of that parameter anyways?  it seems to be another
thing that is not consistent XP.

heh.  that is what this bug is all about.  followLinks has nothing to do with
these predicates.

I will fix up the style nits.
What is nsILocalFile.followLinks for, then?  Is it just for nonterminal links,
in which case, why would anyone ever want it set to false?  Or if (as I hope)
non-terminal links/shortcuts/aliases are always followed, and followLinks
decides whether terminal symlinks are followed, then why shouldn't it affect the
file type predicates?

Say I have an nsIFile instance, and I want to determine its type.  I use the
non-link-following predicates, find that isSymlink returns true, and now I want
to resolve the link to get the true type (or find out whether the link dangles).
 What do I do?

/be
(Reporter)

Comment 20

15 years ago
Comment on attachment 125424 [details] [diff] [review]
patch v.4

sure, we can make it work like that.  

My reservation about doing this -- which lead to the inconsistency -- is what
should we return as the path to this file?  

Should we return the resolve path?  If not, on platforms where symlinks are not
resolve by the kernal, we can find ourselves stating that an nsIFile is one
thing, but the path that we get out of it is something entirely different.
Are we agreed then that all implementations of nsIFile and nsILocalFile must
follow non-terminal symlinks, always?

Can we list the methods and attributes of nsIFile and nsILocalFile that should
always follow terminal symlinks?

What methods and attributes remain, that might be configured by nsILocalFile's
followLinks attribute?

It seems to me we should always associate an nsIFile instance with a particular
pathname, the one with which it was initialized.  Any symlink-resolved pathname
(you might want to resolve all symlinks, non-terminal as well as terminal; you
might want to "squeeze out" /../ and /./ components, which requires non-terminal
symlink resolution) should be recovered from a special attribute or method, a la
realpath(3).  Don't we have such a distinct attribute already?

/be
(Reporter)

Comment 22

15 years ago
here is a quick breakdown of all the of methods (collapsed native methods) in
nsIFile.  This is not how it works today..


String operation - no resolution required
    void append(in AString node);

Should NOT resolve
    attribute AString leafName;
    boolean exists();
    boolean isWritable();
    boolean isReadable();
    boolean isExecutable();
    boolean isHidden();
    boolean isDirectory();
    boolean isFile();
    boolean isSymlink();
    boolean isSpecial();
    attribute unsigned long permissions;
    attribute PRInt64 lastModifiedTime;
    attribute PRInt64 fileSize;
    readonly attribute AString path;
    [noscript] readonly attribute ACString nativePath;

Should resolve
    attribute unsigned long permissionsOfLink;
    attribute PRInt64 lastModifiedTimeOfLink;
    readonly attribute PRInt64 fileSizeOfLink;
    readonly attribute AString target;
    [noscript] readonly attribute ACString nativeTarget;

Dependent on followLinks flag?
    void copyTo(in nsIFile newParentDir, in AString newName);
    void copyToFollowingLinks(in nsIFile newParentDir, in AString newName);

    void moveTo(in nsIFile newParentDir, in AString newName);
    void remove(in boolean recursive);

    void create(in unsigned long type, in unsigned long permissions);
    void createUnique(in unsigned long type, in unsigned long permissions);
    boolean contains(in nsIFile inFile, in boolean recur);
    readonly attribute nsIFile parent;
    readonly attribute nsISimpleEnumerator directoryEntries;

Doesn't matter.
    nsIFile clone();
    boolean equals(in nsIFile inFile);
copyTo and copyToFollowingLinks should do what they say; they shouldn't change
their meaning based on mFollowingLinks, unless our implementations already do in
a way that lots of client code depends on.

In the spirit of O_NOFOLLOW (see open(2) on Linux and FreeBSD), we might respect
mFollowLinks for create.

createUnique and remove should never follow terminal symlinks.  I would argue
the same for moveTo, especially in light of the copyTo/copyToFollowingLinks split.

What to do with parent and directoryEntries?  If they respect mFollowLinks (I'm
calling it by its C++ member name to avoid confusion), then so should the file
type predicates.  Otherwise we haven't provided the lstat vs. stat choice.

It's too bad these interfaces are frozen, with only some "OfLink" and
"FollowingLinks" method variations.  But that leaves us with only a few ways to
go, short of adding an nsIFile2:

1. Clean up inconsistencies among nsIFile and nsILocalFile implementations.
2. Where no {Of,Following}Link{,s}-suffixed variation exists, respect the
setting of mFollowLinks in implementations of nsILocalFile.
3. Document the heck out of this, with concise but complete comments.

/be
(Reporter)

Comment 24

15 years ago
i like 2.  With this flag set to false, should symlinks be allowed in
non-terminal nodes?  

another related option is to stop making windows resolve nodes altogher.  We can
just stop pretending that shortcuts are symlinks (although cygwin bash tries to
do this, users don't do this kind of thing).  With something like this, the
mFollowingLink flag only apply to the terminal node (similar to what
ResolveAlias did on the mac os 7,8,9).  

Updated

15 years ago
Blocks: 201381

Comment 25

15 years ago
speaking of second impls and other flaws...

1. what does IsSpecial mean?

i would have thought that things like /dev/tty* would be special files, but for
windows a special file is just something that's attrib +s. I'm not quite sure
how to get attributes on something like 'con', but i suspect it wouldn't match
the IsSpecial check.

2. (win) nsILocalFile doesn't handle real symlinks:

I:\build>junction link \build

Junction v1.02 - Win2K junction creator and reparse point viewer
Copyright (C) 2000 Mark Russinovich
Systems Internals - http://www.sysinternals.com

Created: I:\build\link
Targetted at: I:\build

I:\build>cd link\link
I:\build\link\link>cd link\link
I:\build\link\link\link\link>t:
T:\mozilla\03061008.moz\bin>xpcshell
js> var
o=Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile)
js> o.initWithPath("i:.\\")
js> o.isDirectory()
true
js> o.isSymlink()
false

I could write code to check for this, but i don't know what values it should
return, the api isn't clear.

Should there be some way for a consumer to be able to detect the difference
between a real symlink and a shortcut file?  note that on windows .lnk isn't
supposed to be the only type of shortcut file, there are at least three in 98,
pif (which xpcom doesn't handle), lnk (which xpcom handles), url (which xpcom
doesn't handle) and others can be configured by setting a flag in the registry.

3. how should an xpcom consumer ask if a nsILocalFile is a mount?
on windows 2000 and beyond mount points can live anywhere on an ntfs file
system, and they are part of the file system not the os's hierarchical system so
like the symlink example above you can have recursive mounts.

There are other attributes in ntfs which also aren't reflected by
nsI(Local)File, files could be compressed or encrypted

4. should xpcom allow you to find out if two files live on the same volume?
unix find is able to limit its traversal to a single volume...

5. _windows Explorer properties_ is smart enough to handle a directory which has
one file 'ICON&#61453;' and two symlinks to the directory. should xpcom be flexible
enough to allow consumers not to end up in silly loops? 

_windows Explorer find_ otoh will traverse through the example in 5 and find
196,605 objects [3*65535] - it doesn't crash. the last item was
I:\Temp\test2\test2\test2\test2\test2\test2\test2\test2\test2\test2\test2\test2\test2\test2\test2
which at 16 deep and 97 long isn't very interesting [i don't know offhand why it
stopped there, it was probably a depth thing].

bookmarks behaves like explorer find (it doesn't crash! but imports the full
empty folder tree - i guess 2*65535, a 13mb temporary bookmarks file)
No longer blocks: 201381
(Reporter)

Comment 26

15 years ago
>speaking of second impls and other flaws...

no one was. :-(

> 2. (win) nsILocalFile doesn't handle real symlinks:

Hence my suggestion to abort our pathetic attempt.  

>how should an xpcom consumer ask if a nsILocalFile is a mount?

> 4. should xpcom allow you to find out if two files live on the same volume?

no.... Not from the nsIFile, nsILocalFile interfaces.  Feel free to propose
another interface that takes a nsILocalFile and returns this kind of information

Updated

15 years ago
Blocks: 201381
1.  We have isSpecial, we should define it as consistenly as possible across
platforms.  We can't remove it.  It would be wrong to make it return false
always on all platforms (Unix in particular).

2.  What kind of file does Junction create?  (It misspells targeted, btw.)  Why
do you think mozilla is doing the wrong thing here?

I:\build>cd link\link
I:\build\link\link>cd link\link
I:\build\link\link\link\link>t:
T:\mozilla\03061008.moz\bin>xpcshell
js> var
o=Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile)
js> o.initWithPath("i:.\\")
js> o.isDirectory()
true
js> o.isSymlink()
false

The current working directory is always a directory, never a symlink (or
shortcut, or any such thing).  When you cd'd through the link, you ended up in a
real, bona fide directory.  Why shouldn't o.isDirectory() return true (and every
other type predicate, especially including isSymlink, return false)?

3.  Why do we care about filesystem mount points?  It's not as if programs want
to know, usually.  Wanting to know about something that's meant to be
transparent is the exception.  Such programs must use other interfaces, native
OS calls, whatever it takes.  They're beyond the purview of XPCOM.

4.  What dougt said (same answer as 3).

5.  nsLocalFileWin.cpp needs to raise ELOOP.  All Unix kernel namei or
pathname-lookup implementations I've seen and worked on detect symlink iloops
and report that error.

/be
(Reporter)

Comment 28

15 years ago
special should remain.  isSpecial, on at least windows, can be also be directory
or file.  special, in this platform's context means that it has the system flag.
 Don't confuse this attribute with a fd pipe.

The more i think about this, the more i want to remove non-terminal shortcuts
from windows.
By "remove non-terminal shortcuts", do you mean following such thinks by reading
them and expanding their contents in the context the non-terminal component has
in its enclosing pathname?

What happens when you then use such a pathname?  Does it work?

If so, then I agree that you should "remove non-terminal component" expansion,
as doing it in nsIFile/nsILocalFile's impl on Windows makes for different
resulting native path of an nsIFile instance than what you'd get (with the
necessary \ for / etc. changes) on Unix.

Unix symlink following is done by the kernel, unless someone calls readlink or a
library routine built on it (e.g., realpath).

If Windows nsIFile is acting like realpath when initializing the instance, that
could be a problem.

But are there programs that barf on non-terminal shortcuts?  Or do all programs
pass pathnames to a layer (OS or usercode) that follows shortcuts, every time
the pathname is used?

/be

Comment 30

15 years ago
it depends what you mean

if you're talking about real symlinks (the junctions i keep complaining about)
then the apps always pass them to the kernel unless they're special (junction
itself).

if you're talking about .lnk files, something usually resolves them early
preventing them from ever being in path strings.
e.g. if i do file>open> and select 'mydoc.html.lnk', the open dialog in any
normal app would resolve it to whereever the lnk pointed, and the app would not
really remember that the user selected a .lnk.

the problem is that we for some reason think having paths with .lnk files in
them is useful. i'm not quite sure why.
(Reporter)

Comment 31

15 years ago
brendan, resolution of non-terminal shortcuts (.lnk files) in the nsIFile
windows implementation is very special to our code.  I do not know any other
apps that handle shortcuts this way.  Even IE will barf on a path with a
non-terminal .lnk file.  removing this support will make the windows
implementation much simpler.  Also, it will make changing the |followLinks|
attribute much less brittle (there can't be an /a/b/c where |b| was a shortcut).

If we fix this bug by (2) "Where no {Of,Following}Link{,s}-suffixed variation
exists, respect the setting of mFollowLinks in implementations of nsILocalFile",
we could make it so that changing |followLinks| was the way to toggle what to do
about the terminal (possiblity making  {Of,Following}Link{,s}-suffixed variation
less important).

So, given: /a/b/c where c is a symlink to /d/e, assuming that these pathes are xp.

nsILocalFile a is inited to /a/b/c with |followLinks| == true.

a.exists() tests for /d/e
a.followLinks = false
a.exists() tests for /a/b/c

Thoughts??
(Reporter)

Comment 32

15 years ago
over to darin.  we have a plan of how this is suppose to work.
Assignee: dougt → darin

Updated

14 years ago
Target Milestone: --- → mozilla1.6beta

Updated

14 years ago
Target Milestone: mozilla1.6beta → mozilla1.7alpha

Updated

14 years ago
Target Milestone: mozilla1.7alpha → mozilla1.7beta

Comment 33

14 years ago
ugh, i need to sit down and think about this one for a while... punting (again) :(
Status: NEW → ASSIGNED
Target Milestone: mozilla1.7beta → mozilla1.8alpha

Updated

14 years ago
Target Milestone: mozilla1.8alpha1 → mozilla1.8beta

Updated

13 years ago
Blocks: 271480

Comment 34

13 years ago
I'm not going to have time for this anytime soon.
Keywords: helpwanted
Target Milestone: mozilla1.8beta1 → Future

Comment 35

12 years ago
Methinks that the appropriate fix is to normalize symlinks in the path part of a filename, but NOT normalize the terminal symlink if:

[ -s $filename ] && [ ! -d $filename ]

An extra check may be needed for OS-X where a directory can be a container for an executable package.
(Reporter)

Comment 36

12 years ago
Comment on attachment 125424 [details] [diff] [review]
patch v.4

clearing old requests
Attachment #125424 - Flags: superreview?(brendan)
Attachment #125424 - Flags: review?(darin)
QA Contact: scc → xpcom

Updated

12 years ago
Assignee: darin → nobody
Status: ASSIGNED → NEW

Updated

9 years ago
Blocks: 484290

Comment 37

7 years ago
Old topic, but the following bug seems to fit here:

Currently checking attributes of dead links (lnk-files, where the file they have been linked to has been deleted) throws an NS_ERROR_FILE_NOT_FOUND, even if you're only checking isSymlink.

Even worse, they are simply omitted when iterating over directoryEntries without any notification. Even more worse, I do have one dead lnk-file that will make hasMoreElements take 3 seconds to proceed and then it will not even be returned by the iterator.

Now that there are no more frozen interfaces in Gecko 2, all the symlink inconsistencies could be resolved in nsIFile.

Cheers.
You need to log in before you can comment on or make changes to this bug.