make nsSafeFileOutputStream follow target symlinks

RESOLVED FIXED

Status

()

Core
Networking: File
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: dwitte@gmail.com, Assigned: dwitte@gmail.com)

Tracking

({fixed-aviary1.0})

Trunk
fixed-aviary1.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
2.85 KB, patch
Biesinger
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
see e.g. bug 206567. there are consumers that save profile files, where the user
might want that file to live somewhere else... so they make the profile file a
symlink. we should follow that symlink when overwriting the target, so that we
don't irritate the user by clobbering their symlink.

we get another nice advantage by following symlinks too - when we move the
tempfile over the target, the move should be atomic, which is safer. (otherwise,
we might be moving a file across filesystems, which might make it less safe.)

patch coming up, with code mostly stolen from timeless ;)
(Assignee)

Comment 1

14 years ago
Created attachment 153622 [details] [diff] [review]
v1
(Assignee)

Comment 2

14 years ago
Comment on attachment 153622 [details] [diff] [review]
v1

timeless, since you're familiar with this... would you mind taking a look? thx!
Attachment #153622 - Flags: review?(timeless)

Updated

14 years ago
Attachment #153622 - Flags: review?(timeless) → review+
(Assignee)

Updated

14 years ago
Attachment #153622 - Flags: superreview?(darin)

Comment 3

14 years ago
Comment on attachment 153622 [details] [diff] [review]
v1

>--- nsFileStreams.cpp	2004-07-18 20:14:14.317769240 -0700

>-    nsCOMPtr<nsIFile> tempResult;
>-    rv = file->Clone(getter_AddRefs(tempResult));
>+    // follow symlinks, for two reasons:
>+    // 1) if a user has deliberately set up a profile file as a symlink, we honor it
>+    // 2) to make the MoveToNative() in Finish() an atomic operation (which may not
>+    //    be the case if moving across directories on different filesystems).
>+    nsCOMPtr<nsIFile> tempResult = do_CreateInstance("@mozilla.org/file/local;1", &rv);
>+    if (NS_SUCCEEDED(rv)) {
>+        nsCOMPtr<nsILocalFile> tempLocal = do_QueryInterface(tempResult);
>+        tempLocal->SetFollowLinks(PR_TRUE);
>+        nsCOMPtr<nsILocalFile> fileLocal = do_QueryInterface(file, &rv);
>+        if (NS_SUCCEEDED(rv))
>+            rv = tempLocal->InitWithFile(fileLocal);
>+    }
>+    // XP_UNIX ignores SetFollowLinks(), so we have to normalize.
>+    if (NS_SUCCEEDED(rv))
>+        rv = tempResult->Normalize();

hmm... why not do this:

    nsCOMPtr<nsIFile> tempResult;
    rv = file->Clone(getter_AddRefs(tempResult));
    // follow symlinks, for two reasons:
    // 1) if a user has deliberately set up a profile file as a symlink, we
honor it
    // 2) to make the MoveToNative() in Finish() an atomic operation (which may
not
    //	  be the case if moving across directories on different filesystems).
    if (NS_SUCCEEDED(rv)) {
	nsCOMPtr<nsILocalFile> tempLocal = do_QueryInterface(tempResult);
	if (tempLocal)
	    tempLocal->SetFollowLinks(PR_TRUE);

	// XP_UNIX ignores SetFollowLinks(), so we have to normalize.
	rv = tempResult->Normalize();
    }

i.e., why do you need to call InitWithFile?  what's wrong with Clone?
(Assignee)

Comment 4

14 years ago
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsILocalFile.idl#102

that claims that one can only SetFollowLinks() on an nsILocalFile that hasn't
been initialized. hence the CreateInstance() and post-links Init(). does Clone()
not constitute "initing the file"?

Comment 5

14 years ago
/me grumbles about nsI*File documentation not corresponding to implementations
(reality).  i'm not sure why dougt thought SetFollowLinks couldn't be called
after a local file is initialized.  that seems silly to me given that we do
indeed seem to support it: by setting a dirty bit that forces us to re-resolve
the target path when needed (XP_WIN case) or by simply re-computing the target
path directly from SetFollowLinks (XP_MACOSX case).  hmm... i think we should
fix the documentation ;-)
(Assignee)

Updated

14 years ago
Attachment #153622 - Flags: superreview?(darin)
(Assignee)

Comment 6

14 years ago
Created attachment 154229 [details] [diff] [review]
v2

ah, neat. in that case, here's your suggestion, and a simple doc change for
nsILocalFile. does this look better?
Attachment #153622 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #154229 - Flags: superreview?(darin)

Comment 7

14 years ago
speaking of sillyness, are we guaranteed that someone won't return 
NS_ERROR_NOT_IMPLEMENTED for normalize if they honor setfollowlinks? :)
(Assignee)

Updated

14 years ago
Keywords: fixed-aviary1.0

Comment 8

14 years ago
Comment on attachment 154229 [details] [diff] [review]
v2

sr=darin

Current implementations of nsIFile::normalize either do something or return
NS_OK.	They don't return NS_ERROR_NOT_IMPLEMENTED.  It seems reasonable to
return NS_OK in the no-op case since there are no required side-effects.  In
other words, the documentation is the existing implementation :-/
Attachment #154229 - Flags: superreview?(darin) → superreview+
(Assignee)

Updated

14 years ago
Attachment #154229 - Flags: review?(timeless)
(Assignee)

Comment 9

14 years ago
Comment on attachment 154229 [details] [diff] [review]
v2

kicking to biesi, coz he likes this stuff ;)
Attachment #154229 - Flags: review?(timeless) → review?(cbiesinger)
Comment on attachment 154229 [details] [diff] [review]
v2

+	 rv = tempResult->Normalize();

do you care whether this fails?
Attachment #154229 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 11

14 years ago
>do you care whether this fails?

probably not... i removed the rv check.

fixed-on-trunk (the rv check is still in aviary, but i'm not going to bother
fixing that.)
(Assignee)

Updated

14 years ago
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.