92329 - GetTarget doesn't handle relative symlinks[FIX][PATCH]

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robin Green]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.2+) Gecko/20010723
BuildID:    2001072308

If I double-click on a symlink which points to a directory in the Save As
dialog, nothing happens - I cannot get in to the directory (via that route).

Reproducible: Always
Steps to Reproduce:
1. Create a symbolic link, e.g. ln -s /home /var/tmp/testlink
2. Load any page
3. Choose Save As from File menu
4. Navigate to symlink
5. Double-click on symlink

Actual Results:  Although the symlink is correctly rendered as a directory,
nothing happens when I double-click on it.

Expected Results:  Should enter the directory.

This happens even with the "simplest" kinds of symlinks - symlinks to
directories in the same directory as the symlink itself.

Obvious workaround: ls -l filename; copy and paste real location into dialog.
However, if you have a lot of symlinks lying around, this workaround can be
annoying.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Probably caused by bug 92294

Comment 2

[Pete Collins (MDG)]

I can't duplicate this on FreeBSD. Everything works as expected.

Maybe someone can apply this patch:

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=43618

From bug#92294 and see if it rectifies this problem for i don't have a linux box
handy any more.

--pete

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

This works for me on linux trunk build 2001072821.  I'm able to save into
symlinked directories.

Comment 4

[Robin Green]

Sorry, this is only for some relative symlinks - absolute symlinks and some
relative symlinks work OK.

My test case was bogus. It should have read:
1. Create a symbolic link, e.g. cd /var/tmp;mkdir t;ln -s t x

Next time I will write EXACTLY what I did! :)

Comment 5

[Pete Collins (MDG)]

Ah, yes, following these new steps, i can reproduce this on unix.

JavaScript error: 
 line 0: uncaught exception: [Exception... "Component returned failure code:
0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsILocalFile.isDirectory]" 
nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS
frame :: chrome://global/content/filepicker.js :: onDblClick :: line 316"  data: no]

Comment 6

[Pete Collins (MDG)]

It's not an nsILocalFile problem. 

js> f;
[xpconnect wrapped nsILocalFile @ 0x81a1900]
js> f.path;
/var/tmp
js> f.append('x');
js> f.path;
/var/tmp/x
js> f.exists();
true
js> f.isDirectory();
true
js> f.isSymlink();
true
js> f.append('foo');
js> f.create(f.NORMAL_FILE_TYPE, 0644);
js> f.path;   
/var/tmp/x/foo
js> f.exists();
true
js> 

Comment 7

[Pete Collins (MDG)]

Attachment: quick patch for filepickler.js

Comment 8

[Pete Collins (MDG)]

greenrd, assign this bug to me since it's not really a filepicker bug.

It actually is an nsILocalFile bug, seems theres a problem w/ symlinks when
initializing w/ initWithUnicodePath.

Looking into it now.

--pete

Comment 9

[Robin Green]

reassigning to pete

Comment 10

[Pete Collins (MDG)]

Attachment: ok, this fixes the problem so ::GetTarget does the right thing

Comment 11

[Pete Collins (MDG)]

The problem was that nsLocalFileUnix::GetTarget wasn't handling relative sym
links correctly. 

GetTarget just wraps libc readlink. So when you have a relative link, readlink
will fill only the relative name into the provided char* buffer. 

So in this situation, target was returning "t", which is the target, but it's
relative. So now everything breaks because "t" is not a full path anymore.

This proposed fix adds some logig to deal w/ the specific situation of readlink
returning relative paths.

cc'ing Brendan and DougT for comments.

If things are cool i will work on getting review and getting this in for 0.9.4.

Can someone take this off of the "UNCONFIRMED" radar?

Thanks

--pete

Comment 12

[timeless]

gladly. sorry about that, it didn't occur to me that you couldn't already... 
granted: canconfirm+editbugs.

Comment 13

[Pete Collins (MDG)]

js> f;  
[xpconnect wrapped nsILocalFile @ 0x81f11c0]
js> f.path;
/var/tmp/x
js> f.isSymlink();
true
js> f.target;
/var/tmp/t
js> o;
[xpconnect wrapped nsILocalFile @ 0x81f1380]
js> o.path;
/home
js> o.isSymlink();
true
js> o.target;
/usr/home/
js> 

Comment 14

[Pete Collins (MDG)]

Attachment: trimed off a few lines of code

Comment 15

[Pete Collins (MDG)]

Attachment: even better . . .

Comment 16

[tenthumbs]

I hope you don't mind a couple of questions. I'm concerned that mozilla
is treating symlinks incorrectly.

First, why is mozilla even looking at the symlink? Since the test link
is pointing at a real directory, statting the link says it's a directory
which should be enough for mozilla.

Second, if I do something like

  #!/bin/sh
  mkdir -p /tmp/foo/bar || exit 1
  mkdir -p $HOME/tmp/foo || exit 2
  rm -f /tmp/frob
  rm -f $HOME/frob
  (cd /tmp/foo/bar; ln -s ../../frob a)
  (cd /tmp; ln -s $HOME/tmp/foo/b frob)
  (cd $HOME/tmp/foo; ln -s ../../frob2 b)
  (cd $HOME; ln -s /tmp/foo frob2)

will the patch allow mozilla to follow /tmp/foo/bar/a properly?

Comment 17

[Pete Collins (MDG)]

The scenario you cite dealing w/ '../../' is another bug 55406 which i also have
a fix for.

I did find a small problem w/ this patch and i will post the fix soon.

Then i will test every conceivable usage of this fix. BUt yes, this is all an
effort to get mozilla dealing properly w/ symlinks.

Thanks

--pete

Comment 18

[Pete Collins (MDG)]

Attachment: using AppendRelativePath instead of Append

Comment 19

[Pete Collins (MDG)]

Attachment: added test to rv on do(QueryInterface

Comment 20

[Brendan Eich [:brendan]]

tenthumbs: agreed (see my comments dated 2001-07-26 10:34 in bug 92294).

petejc: detailed comments below, but I don't see why nsIFile::GetTarget should
return an absolute pathname.  The doc-comment in nsIFile.idl for the target
attribute is woefully incomplete, but it does warn that the result is both
platform-specific and not necessarily sufficient to uniquely identify the file.
 All this suggests to me that you would do better to fix the file picker, if
possible, to form the full path.

Comments:
- Please follow prevailing style by putting a space on either side of = operators:

+        if (NS_FAILED(rv=GetParent(getter_AddRefs(parent))))
+            return rv;

I personally try not to nest assignment, except in loop controls and (sometimes)
in NS_ADDREF actuals.  My 2 cents, but prevailing style may agree.

More important, if you fail here for some unlikely reason, the return rv means
that target is leaked, unless the caller checks and frees it (via XPIDLCString,
we hope) even in the failure-return case.

- You don't need an nsXPIDLCString here:

+        nsXPIDLCString path;
+        path.Adopt(target);
+        if (!path.get())
+            return NS_ERROR_OUT_OF_MEMORY;

Although I guess it helps avoid the leak of target in subsequent return cases.

- Nit: &*pointer is just pointer:

+        if (NS_FAILED(rv=parent->GetPath(&*_retval)))
+            return rv;

But again, I don't see why the target attribute should be absolutized.  Maybe I
am using a dangling symlink on purpose.  Give me the bits from the filesystem,
and let me interpret them.  We need nsIFile semantics to be primitive, or it
won't be sufficient for lots of uses.

/be

Comment 21

[Pete Collins (MDG)]

Using example cited by tenthumbs, and patch from bug 55406 below validates fix.

js> f;
[xpconnect wrapped nsILocalFile @ 0x81f11c0]
js> f.path;
/tmp/foo/bar/a
js> f.isSymlink();
true
js> f.target;
/tmp/foo/bar/../../frob
js> f.exists();
true

Comment 22

[Pete Collins (MDG)]

Brendan, it seems logical for GetTarget to return a full pathname. File picker
is just an example of nsIFile client code expecting the logical result being a
full pathname. 

I agree nsIFile semantics should be primitive. In this case, i don't see the
problem with having a fully resolved pathname on the unix impelemenation.

Can explain a scenario where this may cause a problem or be a bad idea?

--pete

Comment 23

[Brendan Eich [:brendan]]

One scenario is where the link contains a string such as the non-filename value
used in ~/.netscape/lock by 4.x (a host:pid signature).  Primitive means what
you get is what ls -l sees.  If we need a layer or optional method to get the
absolute target, based on the parent directory of the symlink, let's have an
nsIFile.idl extension.

IOW, you're making an incompatible change to an interface that's been around a
while.  I wish dougt or valeski would jump in here.  But apart from the question
of whether nsIFile is "frozen" enough that we shouldn't change semantics of
existing methods, I argue that we need a primitive readlink analogue in the
interface, one that does not absolutize.

/be

Comment 24

[Pete Collins (MDG)]

Attachment: updated patch as to Brendans comments

Comment 25

[Pete Collins (MDG)]

Attachment: filepicker.js patch

Comment 26

[Pete Collins (MDG)]

Both of these patches work and can resolve the cited '/tmp/foo/bar/a/' properly.

I'm only concerned that if we go w/ the filepicker.js patch, more higher level
bugs may creap up in the future due to unclear nsIFile semantics.

So anyway, we have two fixes for this bug. Feel free to pick one and lets get it
checked in when the tree opens.

Thanks

--pete

Comment 27

[Brendan Eich [:brendan]]

Hey jud, is nsIFile going to be frozen?

Pete: I'm wondering if you are willing to come up with a third patch, to extend
(at the end) nsIFile.idl with an absoluteTarget attribute, and with filepicker
changes to use it.

/be

Comment 28

[Pete Collins (MDG)]

Sure, no problem. I think i should file it as a separate bug no?

--pete

Comment 29

[Judson Valeski]

yes, nsIFile will be frozen. See
http://www.mozilla.org/projects/embedding/apiReviewNotes.html#nsIFile for more info.

Comment 30

[Pete Collins (MDG)]

When is the freeze date and when will these changes be made? 
I'll be glad to help out on this.

Two things i disagree w/.

- ditch the char * method versions in favor of unicode versions
- spawn should go away

Comment 31

[Judson Valeski]

pete: freeze date is to be determined :-). any assistance you can provide would
be _much_ appreciated.

these notes were derrived over multiple API review meetings over the past
several months. if you want to start driving on these mods, we can get the
nsIFile owners/implementors/interested parties (as scattered as they are) in a
room and re-visit/re-fine a final plan of attack. drop me some email if you want
to pursue.

Comment 32

[tenthumbs]

Maybe this function should be renamed. Pete Collins looked at its 
internals and treated it as a function to return the next stage in a 
symlink chain. I looked at the name as assumed it was a function to 
return the ultimate target of a symlink chain which is what many system 
calls do.

I wonder what the users of this function think it does? Perhaps that's 
one reason why mozilla has symlink troubles.

I think you really want both functions although a readlink equivalent 
only seems useful in a listing context.

Comment 33

[Pete Collins (MDG)]

tenthumbs, please look at the porkjokeys new group about the upcoming nsIFile
overhaul we need everyone on the same page begfore this is implemented.

Thanks

--pete

Comment 34

[Pete Collins (MDG)]

tenthumbs, BTW, we can implement this to resolve the ultimate target however,
maybe it might be more intuitave to follow the convention of ls -l 

Which in your example would yeild:

$ ls -l /tmp/foo/bar/a
lrwxr-xr-x  1 root  wheel  10 Jul 30 12:32 /tmp/foo/bar/a -> ../../frob

Which this current patch supports as a full path to '../../frob'.

I think if we resolve the ultimate target, we might be breaking convention. I'm
easy either way. 

Thoughts?

Thanks

--pete

Comment 35

[tenthumbs]

Pete, see my comment in n.p.m.porkjockeys. I'm coming to the conclusion that it
is not possible to do the right thing unless you know the user's intent.

I'm not sure that finding the ultimate symlink target is the right thing. I
would never have thought it would ever be necessary until I fiddled with bug
57866. Right now, I'm confused.

Comment 36

[Pete Collins (MDG)]

Attachment: this patch will resolve the ultime path of a symlink chain

Comment 37

[Pete Collins (MDG)]

This has been buggin me some. So i implemented ::GetTarget to resolve the
ultimate path of a symlink. If there is a broken dangling link in the chain, i
beleive we will end there. I need to test that scenario.

Anyway, i wanted to have an implementation around so if we do decide to go this
way, it is here.

This patch is only a first draft.

--pete

Comment 38

[Pete Collins (MDG)]

Here is a bunch of symlinks from singleton to nested chain (using tenthumbs ex)

It works.

js> a.path;
/sys
js> a.target;
//usr/src/sys
js> b.path;
/home
js> b.target;
/usr/home/
js> c.path;
/tmp/foo/bar/a
js> c.target;
/tmp/foo
js> d.path;
/tmp/pete_root
js> d.target;
/D/root/
js> 

Comment 39

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 94290 has been marked as a duplicate of this bug. ***

Comment 40

[Matthias Versen [:Matti]]

*** Bug 94352 has been marked as a duplicate of this bug. ***

Comment 41

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 93029 has been marked as a duplicate of this bug. ***

Comment 42

[Pete Collins (MDG)]

Since it's been agreed that GetTarget should resolve the ultimate target in a
symlink chain, I'd like to check this in.

Can i get some review on this?

Thanks

--pete

Comment 43

[Pete Collins (MDG)]

Ok, if there is a broken link in the chain, this method will return that link.

So, we resolve to the ultimate target or if the chain is broken, the last link
in the chain. 

Sound good?

--pete

Comment 44

[Pete Collins (MDG)]

This will be resolved when bug 94323 is complelted.

--pete

Comment 45

[Pete Collins (MDG)]

I'd like to propose a solution for getTarget that i think is a good one.

Since as part of the nsIFile rework i'm currently doing, most functions will
abide to followLinks. 

What i'm thinking, is for getTarget to abide as well.

So if followLinks is true (the default), we get the realPath of a link or link
chain.
If followLinks is false, we return whatever readlink gives back to us.

In some cases we may have the same path. But if this is clearly documented then
it gives us the option to use either.

Comments?

--pete

Comment 46

[tenthumbs]

I have no inherent objections but I am concerned that this function will
be abused. We may know what symlinks are for but it's clear that not
everyone does.  Are you going to allow high level modules access to
getTarget or will it be (at least partially) hidden?

Also, what are you going to return for a symlink that points to a
non-existent object if followLinks is true?

I have a feeling that "clearly documenting" this will be hard. :-)

Comment 47

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 96068 has been marked as a duplicate of this bug. ***

Comment 48

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 97255 has been marked as a duplicate of this bug. ***

Comment 49

[R.K.Aa.]

*** Bug 98842 has been marked as a duplicate of this bug. ***

Comment 50

[Matthias Versen [:Matti]]

*** Bug 101020 has been marked as a duplicate of this bug. ***

Comment 51

[Jesse Ruderman]

*** Bug 101349 has been marked as a duplicate of this bug. ***

Comment 52

[dylang]

Question: what's going on with this bug?  Are we progressing?  Will Moz work or
not work with my symlinks?

Comment 53

[Pete Collins (MDG)]

I fixed this bug, but the fix is part of a large neIFile change that won't be
landing anytime soon unfortunately. I have been tied up w/ contract work for the
moment and had to stop working on this. I hope to resume w/ bug 94323 in a week
or two. Or at least take the implementation for this fix and get it into the
trunk before the big chage lands. 

But yes mozilla does the right thing w/ symlinks now. So try to hold on.
Please track 94323 for any updates on progress. I beleive Conrad is driving the
module in my absense.

--pete

Comment 54

[Alfonso Martinez]

*** Bug 101944 has been marked as a duplicate of this bug. ***

Comment 55

[Matthias Versen [:Matti]]

*** Bug 103092 has been marked as a duplicate of this bug. ***

Comment 56

[Pete Collins (MDG)]

pusing off

Comment 57

[Greg Onufer]

Reproducible on Sun/SunOS, this isn't restricted to PC/Linux.
Nor is it limited to relative symlinks.

Comment 58

[Matthias Versen [:Matti]]

*** Bug 104749 has been marked as a duplicate of this bug. ***

Comment 59

[Boris Zbarsky [:bzbarsky]]

*** Bug 105969 has been marked as a duplicate of this bug. ***

Comment 60

[R.K.Aa.]

*** Bug 105993 has been marked as a duplicate of this bug. ***

Comment 61

[Pete Collins (MDG)]

I would like to check in patch #44746. For it fixes this problem once and for
all against the current implementation of nsIFile. 

This can preimplement the ultimate but lengthy changes comming down the pike for
nsIFile which will land in various stages. 

Can i please have r= and sr=.

Thanks

--pete

Comment 62

[Conrad Carlen (not reading bugmail)]

+            nsCOMPtr<nsILocalFile>
localFile(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv));
+            if(NS_FAILED(rv))
+                return rv;
+            if (NS_FAILED(rv = localFile->InitWithPath(target)))

You can avoid a trip through the component mgr by using:
nsCOMPtr<nsILocalFile> localFile;
rv = NS_NewLocalFile(target, PR_TRUE, getter_AddRefs(localFile));

When the nsIFile API is reworked, I'd expect this:
GetTarget() would be gone.
GetPath() would observe the state of followLinks. If TRUE, and the file is a
symLink, it would do what this patch does. Otherwise, it returns the raw path. 

Comment 63

[Pete Collins (MDG)]

Attachment: using NS_NewLocalFile

Comment 64

[Pete Collins (MDG)]

I agree, GetTarget would stay around as an nsresult to be implemented by GetPath
and should be pulled out of the interface.

--pete

Comment 65

[Alfonso Martinez]

*** Bug 107508 has been marked as a duplicate of this bug. ***

Comment 66

[Conrad Carlen (not reading bugmail)]

Comment on attachment 55532 [details] [diff] [review]
using NS_NewLocalFile

r=ccarlen

Comment 67

[Pete Collins (MDG)]

Brendan, can i get an sr= here

--pete

Comment 68

[Pete Collins (MDG)]

Doug, i need sr= for patch #55532.

The last one on the list.

Thanks

--pete

Comment 69

[Brendan Eich [:brendan]]

As I wrote in mail to pete, I'm sr'ing the latest patch (dougt is not a
super-reviewer, although darin delegates to him for necko stuff).  Please be
patient, I'll have comments posted within an hour.

/be

Comment 70

[Brendan Eich [:brendan]]

Comment on attachment 55532 [details] [diff] [review]
using NS_NewLocalFile

Nit: combine ifs using &&:

+            if (!parent) 
+                if (NS_FAILED(rv = GetParent(getter_AddRefs(parent))))
+                    return rv;

(May ward off dangling-else later.)

Nit: space after if:

+            if(NS_FAILED(rv))
+                return rv;
+            if (NS_FAILED(rv = localFile->AppendRelativePath(target)))

Ok, the while (isSymLink) loop bugs me: it does a bunch of work after
testing isSymLink at the top (initialized to true), then it tests again
after computing a new isSymLink, parent, and _retval and does more work if isSymlink.
I think it should be a for (;;) {...} loop that breaks if (!isSymlink),
as soon as it has computed the new isSymlink.

I need to study it more, but wanted to give feedback now.  I'll look at a
new patch as soon as I see the bugmail.

/be

Comment 71

[Pete Collins (MDG)]

Attachment: changing while loop to a for loop

Comment 72

[Pete Collins (MDG)]

The flavor of which loop is implemented is really "6 in one, half a dozen in the
other". You still get the same results using the same amount of cycles.

The way this algorithm works . . .

First we check for a relative target path target[0] != '/'
If so, we create a new local file and see if it is a symlink.

Otherwise the target is an absolute path and we create a local file from that
and test if it's a link.

Then we proceed to do a read link inside the loop until we finally reach the
ultimate target, a nonlink file or dir.

This patch has been well tested and always yeilds the expected logical results
.

--pete

Comment 73

[Brendan Eich [:brendan]]

Comment on attachment 56069 [details] [diff] [review]
changing while loop to a for loop

pete, sorry to pick nits, I'm a stickler.  My point was not just the test-at the top
(or the remaining unnecessary initialization of isSymLink).
It was also that you don't need to call GetParent in two places
(code bloat).
Call it after the if (!isSymLink) break; and set parent = do_QueryInterface(localFile)
in the else block, so
you can use parent uniformly after the break.

Why do we strip only one trailing / from target? Could there be more than one?

/be

Comment 74

[Pete Collins (MDG)]

Attachment: pulled getParent out of the loop, added check for multiple trailing '/' s

Comment 75

[Pete Collins (MDG)]

Brendan, I can't call GetParent after the isSymlink break, both blocks above
will yeild different results. They are not the same.

What you are suggesting is wrong, there are two distinctly different parents
obtained from each block.

I'm sticking with this latest patch.

--pete

Comment 76

[Brendan Eich [:brendan]]

Comment on attachment 56126 [details] [diff] [review]
pulled getParent out of the loop, added check for multiple trailing '/' s

Pete: what I meant was, instead of this:

+            if (NS_FAILED(rv = parent->GetParent(getter_AddRefs(parent))))
+                return rv;
+        } else {
+            nsCOMPtr<nsILocalFile> localFile;
+            rv = NS_NewLocalFile(target, PR_TRUE, getter_AddRefs(localFile));
+            if (NS_FAILED(rv))
+                return rv;
+            if (NS_FAILED(rv = localFile->IsSymlink(&isSymlink)))
+                return rv;
+            if (NS_FAILED(rv = localFile->GetParent(getter_AddRefs(parent))))
+                return rv;
+            *_retval = target;
+        }
+        if (!isSymlink)
+            break;

do this:

+        } else {
+            nsCOMPtr<nsILocalFile> localFile;
+            rv = NS_NewLocalFile(target, PR_TRUE, getter_AddRefs(localFile));
+            if (NS_FAILED(rv))
+                return rv;
+            if (NS_FAILED(rv = localFile->IsSymlink(&isSymlink)))
+                return rv;
+            parent = do_QueryInterface(localeFile);
+            *_retval = target;
+        }
+        if (!isSymlink)
+            break;
+        if (NS_FAILED(rv = parent->GetParent(getter_AddRefs(parent))))
+            return rv;

See what I mean?

/be

Comment 77

[Pete Collins (MDG)]

Attachment: moved breaks above getParent

Comment 78

[Brendan Eich [:brendan]]

Comment on attachment 56193 [details] [diff] [review]
moved breaks above getParent

Ok, no problem -- two 'if (!isSymLink) break;' occurrences (didn't you like
my 'parent = do_QueryInterface(localFile)' hack?).

This final paragraph is leaky, isn't it?

+        while (target[len-1] == '/' && len > 1)
+            target[--len] = '\0';
+        if (*_retval == target) 
+            *_retval = ToNewCString(nsDependentCString(target, len));
+        lstat(*_retval, &symStat);
+        if (!S_ISLNK(symStat.st_mode))
+            return NS_ERROR_FILE_INVALID_PATH;
+        if (!target)
+            return NS_ERROR_OUT_OF_MEMORY;
+        size = symStat.st_size;
+        if (readlink(*_retval, target, size) < 0) {
+            nsMemory::Free(target);
+            return NSRESULT_FOR_ERRNO();
+        }
+        target[size] = '\0';

Also, the null target test leading to NS_ERROR_OUT_OF_MEMORY return
makes no sense -- target was dereferenced just above in the while loop.

/be

Comment 79

[Pete Collins (MDG)]

Attachment: freeing allocated buffer

Comment 80

[Pete Collins (MDG)]

Yes, once again being a child corrupted by those damn runtime languages which do
all that memory stuff for you . . .

I am null testing both buffers after they have been allocated for out of memory.

--pete

Comment 81

[Pete Collins (MDG)]

Attachment: pulling the target null test

Comment 82

[Brendan Eich [:brendan]]

Comment on attachment 56199 [details] [diff] [review]
pulling the target null test

+        if (*_retval == target) 
+            *_retval = ToNewCString(nsDependentCString(target, len));
+        if (!*_retval)
+            return NS_ERROR_OUT_OF_MEMORY;

Test !*_retval only if you called ToNewCString:

    if (*_retval == target) {
        *_retval = ToNewCString(nsDependentCString(target, len));
        if (!*_retval)
            return NS_ERROR_OUT_OF_MEMORY;
    }

+        lstat(*_retval, &symStat);
+        if (!S_ISLNK(symStat.st_mode))
+            return NS_ERROR_FILE_INVALID_PATH;

Doesn't this return right above leak target/*_retval?

The readlink error return also will leak *_retval, won't it?

Maybe it's time for an nsXPIDLCString or two here!

/be

Comment 83

[Pete Collins (MDG)]

Attachment: fixing leaks

Comment 84

[Pete Collins (MDG)]

Brendan, i'm sticking w/ using *_retval and target and not introducing any new
types.

I am freeing up them before any failure returns.

So i *hope* am am free of leaks now.

--pete

Comment 85

[Brendan Eich [:brendan]]

Comment on attachment 56246 [details] [diff] [review]
fixing leaks

Just noticed this:

-    *_retval = nsCRT::strdup((const char *)mPath);
+    *_retval = nsCRT::strdup(mPath.get());

in GetPath, but that should use the standard allocator (nsMemory::Clone), not
nsCRT::strdup, which uses PL_strdup and must be matched by nsCRT::free.

I don't see why you're duplicating *_retval if it == target -- why not
just return target and not free it in that case, and avoid a
useless copy?

/be

Comment 86

[Pete Collins (MDG)]

Attachment: pulling copy

Comment 87

[Pete Collins (MDG)]

BTW: Brendan, there are other instances of this type of `nsCRT::strdup' useage.
I'm not going to address them, they are beyond the scope of this bug. 

Especially since this patch is ready to be checked in, right?

sr me baby!

;-)

--pete

Comment 88

[tenthumbs]

OK, I'm not objecting to anything in the latest patch but should we be 
concerned about the ad hoc approach to path canonicalization? It 
probably always works here but maybe it should be centralized somewhere.

Comment 89

[Pete Collins (MDG)]

Yea, i was thinking the same thing. Perhaps a macro or something to clean path
strings used in libc calls like in the case of this patch.

There is another bug i filed to clean up the existing implementations for
nsLocalFileUnix which can include this type of cleanup as well.

Remember this method will eventually be pulled out of the idl interface and used
only in the class implementations.

Like Conrad mentioned, GetPath will obey followlinks, so there won't be a need
for getTarget anymore. 

--pete

Comment 90

[Pete Collins (MDG)]

I hope I can check this in and done with before the freeze tomorrow night.

--pete

Comment 91

[Florian Brunner]

I still have this bug with build 2001101202 on SuSE Linux 7.1 (kernel 2.2.18;
KDE 2.2).
Links like:
data -> /windows/e/Data/
windows -> /windows/

The problem occured in the File|Open File-Dialog.

Cheers
Florian

Comment 92

[Pete Collins (MDG)]

Once i check this patch in it will be fixed. I'm hoping to land this before the
freeze for 0.9.6.

Just waiting for an sr= from Brendan.  ;-)

--pete

Comment 93

[Brendan Eich [:brendan]]

Comment on attachment 56271 [details] [diff] [review]
pulling copy

Sorry, still leaks.  My version coming right up.

/be

Comment 94

[Brendan Eich [:brendan]]

Attachment: don't leak *_retval or target after error, or target after success if it was relative

Comment 95

[Brendan Eich [:brendan]]

Attachment: old-on-left, new-on-right plain diff of pete's last patch vs. mine

Comment 96

[Brendan Eich [:brendan]]

The last attachment, an old-style diff of attachment 56271 [details] [diff] [review] ("pulling copy")
against attachment 56626 [details] [diff] [review] ("don't leak *_retval...") should make clear what's
going on.  In particular, notice that all the early return rv; statements after
failures are now breaks.  Also notice how *_retval is kept null at loop body top
(first time through, or when looping because isSymLink).  Finally note that
target must be freed even on success, at the bottom, if it does not equal
*_retval.  And *_retval if non-null must be freed after failure, again at the
bottom to handle all the failure cases in the loop.

Please review and test, I'll sr when someone r='s and several people testify
that the patch works well in many cases.

/be

Comment 97

[Pete Collins (MDG)]

Stellar! Duh, *mental not*, it is stupid to return from a loop and cause leaks
like this.

dont_QueryInterface, i never heard of it . . .  

Hey, i'll be coding like this some day.  ;-)

I see one problem though, rv is dead ended. 

patch forthcoming . . .

--pete

Comment 98

[Pete Collins (MDG)]

Attachment: returning rv instead of NS_OK

Comment 99

[Pete Collins (MDG)]

Attachment: pulling out initializer don't need it

Comment 100

[Brendan Eich [:brendan]]

Comment on attachment 56646 [details] [diff] [review]
pulling out initializer don't need it

Pete, thanks for wiping my patch's chin.  sr=[e-mail].

/be

Comment 101

[Pete Collins (MDG)]

Checked in. Brendan, Thanks for the killer help here!

It would be nice if the file picker added a ticker to dirs that are symlinks.
That would be easy to implement.

Marking fixed, will need to be verified by someone.


--pete

Comment 102

[David Tangye]

Hey wait up folks. I dunno if it has been missed so far, but 0.9.5 DOES work
fine if you slect a link and click the button, which has the label "Open" set
when you have selected a link or subdirectory. It seems to be only the
double-click that does not work. So the problem must be in how the app is
architected, in OO terms both user actions should trigger the same event/pass
the same params to the same target (object).

You have now indicated to have fixed the bug for 0.9.7. Yet the discussions I
have seen seemed to me that you were focussed deeper into the lower levels of
the file system, which seemed worrying to me. I trust I worry needlessly. Anyway
I am reopening this in case. Close it right up again if I am outta line.

OK, so maybe this is not the place to raise this but: Something tells me from
this and numerous other bugs I have seen that there is much code-hackery in the
linux world and litle understanding of OO design concepts, else this sort of
error would not occur.

Comment 103

[Asa Dotzler [:asa]]

This bug is fixed. If you have specific functional or code issues please open a
new bug. 

Comment 104

[David Tangye]

OK, so close it again. My previous message is merely to question whether you
have actually fixed the problem.

It seems to me that the software might be becoming a mess of hacks, given the
messages in this and other threads. But I guess there is not real way to address
this potentially big picture issue using Bugzilla, which helps hackers focus on
the specifics of one bug or bug-group, not the big-picture.

Comment 105

[Pete Collins (MDG)]

Dave, this is now fixed. I checked it into the trunk yesterday. It will be part
of the branch cut for 0.9.6 tonight. This is fixed and i can verify it myself.

As far as the hacking goes. The only hacking here was in my patch. But hey, i'm
a hacker. Brendan helped me fix it up before letting me check it in. So this bug
is a text book example of how well code is reviewed before it goes into the tree. 

The problem you saw at the GUI level was caused at the lower local filesystem
implementation level.
Which i now fixed. Even though it appears to be an event/GUI problem.

To see this working, you would need to pull the trunk and compile mozilla or
just wait for 0.0.6 or a nightly to come out.

I'm marking verified since i duplicated the reporters steps and now get the
desired behavior.

I'm a big picture guy myself and we finally tightened that picture a bit by
getting down to the nitty gritty here in this bug.

;-)

--pete

Comment 106

[R.K.Aa.]

*** Bug 109548 has been marked as a duplicate of this bug. ***

Comment 107

[R.K.Aa.]

*** Bug 109879 has been marked as a duplicate of this bug. ***

Comment 108

[Boris Zbarsky [:bzbarsky]]

*** Bug 110066 has been marked as a duplicate of this bug. ***

Comment 109

[R.K.Aa.]

*** Bug 112300 has been marked as a duplicate of this bug. ***

Comment 110

[R.K.Aa.]

*** Bug 116195 has been marked as a duplicate of this bug. ***