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

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jag (Peter Annema), Assigned: jag (Peter Annema))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Created attachment 369471 [details] [diff] [review]
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.
Assignee: nobody → jag
Attachment #369471 - Flags: superreview?(benjamin)
Attachment #369471 - Flags: review?(neil)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
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.
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
Created attachment 369484 [details] [diff] [review]
Handle links to links and dangling links cleanly, v2

Address comments, move things around a bit.
Attachment #369471 - Attachment is obsolete: true
Attachment #369484 - Flags: superreview?(benjamin)
Attachment #369484 - Flags: review?(neil)
Attachment #369471 - Flags: superreview?(benjamin)
Attachment #369471 - Flags: review?(neil)

Updated

9 years ago
Attachment #369484 - Flags: review?(neil) → review+

Updated

9 years ago
Attachment #369484 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 6

9 years ago
http://hg.mozilla.org/mozilla-central/rev/f1d4575ba13e
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.