Closed
Bug 485328
Opened 15 years ago
Closed 15 years ago
nsLocalFileUnix::GetNativeTarget() is broken for symlinks and (dangling) symlinks
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jag+mozilla, Assigned: jag+mozilla)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
4.23 KB,
patch
|
neil
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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•15 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•15 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•15 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•15 years ago
|
||
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•15 years ago
|
Attachment #369484 -
Flags: review?(neil) → review+
Updated•15 years ago
|
Attachment #369484 -
Flags: superreview?(benjamin) → superreview+
Assignee | ||
Comment 6•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f1d4575ba13e
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•