252050 - make nsSafeFileOutputStream follow target symlinks

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[[e-mail]]

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 ;)

Comment 1

[[e-mail]]

Attachment: v1

Comment 2

[[e-mail]]

Comment on attachment 153622 [details] [diff] [review]
v1

timeless, since you're familiar with this... would you mind taking a look? thx!

Comment 3

[Darin Fisher]

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?

Comment 4

[[e-mail]]

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

[Darin Fisher]

/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 ;-)

Comment 6

[[e-mail]]

Attachment: v2

ah, neat. in that case, here's your suggestion, and a simple doc change for
nsILocalFile. does this look better?

Comment 7

[timeless]

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

Comment 8

[Darin Fisher]

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 :-/

Comment 9

[[e-mail]]

Comment on attachment 154229 [details] [diff] [review]
v2

kicking to biesi, coz he likes this stuff ;)

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 154229 [details] [diff] [review]
v2

+	 rv = tempResult->Normalize();

do you care whether this fails?

Comment 11

[[e-mail]]

>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.)