485328 - nsLocalFileUnix::GetNativeTarget() is broken for symlinks and (dangling) symlinks

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[jag (Peter Annema)]

When we're resolving a link to a link, we need to make sure the |target| buffer is large enough for the second link.

Also, for a dangling symlink, without the fix for bug 485325 isSymlink will sometimes be true (yay garbage) so we'll fall through to the lstat() to double-check we're a symlink, and break out there with an error. With the fix for that bug we'll break out on one of the IsSymlink() calls with an error.

Once bug 485325 is fixed I think we should take any failure from either IsSymlink() to be the signal that we've gone as far as we can, and return the path string we've built so far as the target. Patch coming up.

Comment 1

[jag (Peter Annema)]

Attachment: Handle links to links and dangling links cleanly

Bonus: drop lstat64 for FileSizeOfLink() (no symlink is going to be more than 2GB), and make sure we don't get stuck in an infinite loop.

Comment 2

[[e-mail]]

Comment on attachment 369471 [details] [diff] [review]
Handle links to links and dangling links cleanly

>+            localFile = do_QueryInterface(parent, &rv);
>             if (NS_FAILED(rv))
>                 break;
>+            NS_ASSERTION(localFile, "no parent?!");
do_QueryInterface can't succesfully return null.

>+            if (NS_FAILED(rv = localFile->AppendRelativeNativePath(nsDependentCString(target, size))))
This is wrong, size is the size of the buffer, not the link.

Comment 3

[jag (Peter Annema)]

Comment on attachment 369471 [details] [diff] [review]
Handle links to links and dangling links cleanly

>+    PRInt32 maxLinks = 40;
>+    while (PR_TRUE) {
>+        if (maxLinks-- == 0) {
>+            rv = NS_ERROR_FAILURE;

rv = NS_ERROR_FILE_UNRESOLVABLE_SYMLINK


And yeah, I even realized that size would be the size of the buffer, not the file (which means I'm also putting the zero terminator in the wrong spot), and then I forgot about it.

And I'll remove the useless assertion.

Comment 4

[jag (Peter Annema)]

Oh, and on irc Neil made a case that currently people might be using GetFileSizeOfLink() the way lstat is used on Unix, so I should keep the lstat64() call there.

Comment 5

[jag (Peter Annema)]

Attachment: Handle links to links and dangling links cleanly, v2

Address comments, move things around a bit.

Comment 6

[jag (Peter Annema)]

http://hg.mozilla.org/mozilla-central/rev/f1d4575ba13e