82205 - nsLocalFile::IsSpecial (Unix) is broken [PATCH]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[tenthumbs]

This bug comes from a discussion in bug 81927.

In mozilla/xpcom/io/nsLocalFileUnix.cpp, the nsLocalFile::IsSpecial uses
S_ISLNK(mCachedStat.st_mode). Since mCachedStat appears to be the result
of a stat system call, not lstat, S_ISLNK will always fail.

Now it is possible to "fix" this by calling lstat in this method, but I
think that's exactly the wrong thing to do. One almost never, except in
a listing context, needs to know whether a file is a symlink. It's the
target that's important. For example, if I symlinked ~/.signature to
some regular file I would expect mozilla to use that file. If I
symlinked the cache directory to another director on another disk with
lots of space so I can have a 1GB cache, that should also work.

In addition, there's a quite nice IsSymlink method available.

For these reasons, the attached patch just removes S_ISLNK.

Now it is also true that lxr doesn't show any place where IsSpecial is
actually used but it is exposed in libxpcom.so so it's not like this can
be totally ignored.

Comment 1

[tenthumbs]

Crap! After 5 minutes, the attachment POST timed out. Stupid bugzilla. Here's th
patch. I create an attachment when bugzilla feels better.

--- mozilla/xpcom/io/nsLocalFileUnix.cpp.orig   Tue May 22 13:53:16 2001
+++ mozilla/xpcom/io/nsLocalFileUnix.cpp        Tue May 22 16:00:49 2001
@@ -1161,8 +1161,7 @@
 {
     NS_ENSURE_ARG_POINTER(_retval);
     VALIDATE_STAT_CACHE();
-    *_retval = !S_ISLNK(mCachedStat.st_mode) &&
-               !S_ISREG(mCachedStat.st_mode) &&
+    *_retval = !S_ISREG(mCachedStat.st_mode) &&
                !S_ISDIR(mCachedStat.st_mode);
     return NS_OK;
 }

Comment 2

[tenthumbs]

Attachment: patch

Comment 3

[John Bandhauer]

No comment from bug owner?

Comment 4

[Doug Turner (:dougt)]

Edward, the patch looks okay to me... a link really isn't a special.  What is 
holding this up?

Comment 5

[Edward Kandrot]

The hold up would be an r=, sr=, and a=.  I do not know what the ramifications
of this change would be, though there does not seem to be any.  If I knew this
code, I would have given an r=.  Can the hold up be by-passed by someone who
knows the code giving it an r=, then sr=, then I will submit it for a=?  Thanks.

Comment 6

[tenthumbs]

Perhaps it is better to detect what a file is rather than what it is not. New
patch upcoming.

Comment 7

[tenthumbs]

Attachment: new patch detects what is

Comment 8

[Pete Collins (MDG)]

Attachment: updating patch

Comment 9

[Pete Collins (MDG)]

js> o.path;
/dev/zero
js> o.isFile();
false
js> o.isDirectory();
false
js> o.isSymlink();
false
js> o.isSpecial();
true
js> 

tested and looks good to me.

r=pete

Lets get this in.  Someone want to sr= this bug?

--pete

Comment 10

[Pete Collins (MDG)]

tenthumbs mark this to milestone to 0.9.3.
Lets get all these nsLocalFile fixes in to this target.

--pete

Comment 11

[tenthumbs]

Ok, I'll risk Mozilla's wrath. Changing milestone.

Comment 12

[Johnny Stenback (:jst)]

Looks good to me, sr=jst

Comment 13

[Pete Collins (MDG)]

tenthumbs, do you want to check this in? 
We are a go . . .

--pete

Comment 14

[Pete Collins (MDG)]

Provided the tree is open . . .

;-)

--pete

Comment 15

[tenthumbs]

I don't have checkin privileges so someone else will have to do the deed.

Comment 16

[Pete Collins (MDG)]

checked in please resolve this to fixed

Thanks

--pete

Comment 17

[tenthumbs]

Resolving fixed.

Comment 18

[Makoto Hamanaka]

pete, BeOS build is broken now because there is no S_ISSOCK in BeOS.
In BeOS, a file can't be a socket. So please do like this.

#if !defined(XP_BEOS)
  *_retval = S_ISCHR(mCachedStat.st_mode)  ||
  S_ISBLK(mCachedStat.st_mode)  ||
  S_ISFIFO(mCachedStat.st_mode) ||
  S_ISSOCK(mCachedStat.st_mode);
#else
  *_retval = S_ISCHR(mCachedStat.st_mode) ||
  S_ISBLK(mCachedStat.st_mode) ||
  S_ISFIFO(mCachedStat.st_mode);
#endif

Comment 19

[Makoto Hamanaka]

reopen this bug.

Comment 20

[tenthumbs]

Attachment: patch against current version

Comment 21

[tenthumbs]

Attachment: patch against current version

Comment 22

[tenthumbs]

Crap! New patch against current version. Take the last one. Twitchy thumbs on my
part made a double attachment.

Comment 23

[tenthumbs]

Someone should note somewhere that BEOS isn't POSIX compliant.

Comment 24

[Mike Kaply [:mkaply]]

I checked in the BeOS fix as a build bustage fix.

Comment 25

[Pete Collins (MDG)]

Mike, thanks. I just got back online today due to upgrades i've been doing to my
home network. 

Sorry for the delayed response and thanks for ckecking in the fix Mike.

--pete

Comment 26

[Makoto Hamanaka]

Thanks!