81927 - nsIFile isSymlink() is broken [PATCH]

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Pete Collins (MDG)]

nsIFile isSymlink() is broken. 

Attaching patch 

--pete

Comment 1

[Pete Collins (MDG)]

Attachment: patch to fix broken implementation of isSymlink() method

Comment 2

[Pete Collins (MDG)]

Output of test to verify fix:


js> var p='/tmp/foo_link';
js> const File=new Components.Constructor("@mozilla.org/file/local;1",
"nsILocalFile");
js> var f=new File();
js> f.initWithPath(p);
js> f.exists();
true
js> f.path;
/tmp/foo_link
js> f.isSymlink();
true
js> p='/tmp/foo';
/tmp/foo
js> f.initWithPath(p);
js> f.exists();
true
js> f.path;
/tmp/foo
js> f.isSymlink();
false
js>

Comment 3

[Pete Collins (MDG)]

This method has been broken for as long as i can remember.

The problem it seems that in order for S_ISLNK() to work, you need to initialize
the stat struct w/ lstat() not stat.

--pete

Comment 4

[tenthumbs]

Wouldn't it be better to redefine S_ISLNK? For example, look at isSpecial just
below isSymlink.

I suggest extreme caution here. You generally do NOT want to know if a file is
a symlink.

Comment 5

[Pete Collins (MDG)]

> wouldn't it be better to redefine S_ISLNK? For example, look at isSpecial just
> below isSymlink.


S_ISLNK is a stdlib call which looks like it is being used wrong in isSpecial.

!S_ISLNK(mCachedStat.st_mode)

This will never return true if initialized w/ stat() instead of lstat() 

> I suggest extreme caution here. You generally do NOT want to know if a file is
> a symlink.

I'm not sure i understand? There is a member method called isSymlink() which
returns a bool based on if the file is a symlink or not.
It is broken. This patch fixes it . . .

What is there to be cautionary about a method that is specifically designed to
inform if the file is a symlink or not?

--pete

Comment 6

[tenthumbs]

I thought mozilla had redefined S_ISLNK somewhere. I guess I'm wrong.

What concerns me is that the procedure in IsSymlink will be replicated
in other places in teh code where it will, almost always, be the wrong
thing to do. Look at the IsSpecial method just below IsSymlink. the
S_ISLNK test doesn't belong there at all. You care whether the link
target is a special file, not the link itself.

Would you consider adding a comment in the code about why IsSymlink is
special?

I'll file another bug about IsSpecial.

Comment 7

[Pete Collins (MDG)]

I think ::IsSpecial should be implemented something like this:

NS_IMETHODIMP
nsLocalFile::IsSymlink(PRBool *_retval)
{
    NS_ENSURE_ARG_POINTER(_retval);
    VALIDATE_STAT_CACHE();
    CHECK_mPath();

    struct stat symStat;
    lstat(mPath, &symStat);
    *_retval=S_ISLNK(symStat.st_mode);
    return NS_OK;
}


I can post that patch if you open up another bug.

--pete

Comment 8

[Pete Collins (MDG)]

Duh, sorry 

like this:

NS_IMETHODIMP
nsLocalFile::IsSpecial(PRBool *_retval)
{
    NS_ENSURE_ARG_POINTER(_retval);
    VALIDATE_STAT_CACHE();
    CHECK_mPath();

    struct stat symStat;
    lstat(mPath, &symStat);

    *_retval = !S_ISLNK(symStat.st_mode) &&
               !S_ISREG(mCachedStat.st_mode) &&
               !S_ISDIR(mCachedStat.st_mode);
    return NS_OK;
}

Comment 9

[tenthumbs]

I filed bug 82205 on IsSpecial but I took a completely opposite approach to
yours. Feel free to call me an idiot.

Comment 10

[Edward Kandrot]

Thanks for the patch.  It appears to be a simple fix, though I do not know this
code, nor why it did not work in the first place.  Is there a way within the
product to verify that this works?  Is there code that depends on this being
"broken"?  Thanks.

Comment 11

[Pete Collins (MDG)]

> Is there a way within the product to verify that this works?  

I posted the output of a js test that validates the fix.
Just run it in xpcshell youself to verify . . .

> Is there code that depends on this being "broken"? 

Heh, i guess a search in lxr would show you client code using this method.

--pete

Comment 12

[Pete Collins (MDG)]

Attachment: removed validate cache call

Comment 13

[Pete Collins (MDG)]

Attachment: really removed now

Comment 14

[Pete Collins (MDG)]

you can assign this bug to me if you want, i will be maintaining this patch

--pete

Comment 15

[Pete Collins (MDG)]

This patch still works and doesn't need updating

--pete

Comment 16

[Pete Collins (MDG)]

Taking bug.

--pete

Comment 17

[Christopher Blizzard (:blizzard)]

r=blizzard

Comment 18

[Johnny Stenback (:jst)]

sr=jst

Comment 19

[Pete Collins (MDG)]

checked in this mornng. Marking fixed.

--pete