760937 - Symlinks are dereferenced in Linux in "Local Directory" setting

Status: ASSIGNED

(MailNews Core :: Account Manager, defect)

Description

[rmhristev]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120424151700

Steps to reproduce:

I moved my home dir to a new disk. Symlink was redirected to new disk, i.e moved /home/account -> old_disk to /home/account -> new_disk


Actual results:

Account setting -> Server Settings -> Local Directory pointing to old _DE_referenced link
Folders inaccessible.No error message issued. The account was just without any folders. Subscription requests were shown but ignored.


Expected results:

Application should always use the symlink. Symlinks are supposed to mask underlying hardware and should not be dereferenced by the application, only by the system at the time of call.

Strangely enough it appears that the Gmail accounts seems to have fixed themselves, but the regular IMAP accounts got broken.

At worst: give at least an error message, no error message was issued for non-existent path.

Comment 1

[:aceman]

David Bienvenu, any idea how to prevent this?

Comment 2

[rmhristev]

"Attach signature from a file" seems to be in the same boat.

Comment 3

[David :Bienvenu]

(In reply to :aceman from comment #1)
> David Bienvenu, any idea how to prevent this?

It looks to me on Linux nsIFile always follows links. Is the pref that's stored for local directories or signatures the unresolved link or the resolved path? It looks to me like we would need to go to a bit of trouble to get the resolved path from an nsIFile, but I don't have linux to test with here.

Comment 4

[:aceman]

I understood from the reporter that the pref seems to store the resolved path.

But I have not yet confirmed it so I'll look at it on Linux.

Comment 5

[:aceman]

David, I can confirm this. I noticed I also have the profile in a symlinked directory.
1. So I looked into prefs.js and really, all mail.server.server*.directory and mail.server.server*.directory-rel were dereferenced into the full path.
2. So I made an experiment. I edited one of the prefs to contain the path using the symlink.
3. TB started fine and picked up the folders, showed mail in that account correctly.
4. When closing TB now, the path in prefs.js was unchanged (kept the symlink).
5. Then after next start of TB I went into Account manager and voila, the path in the Local Folders field appeared dereferenced (without symlink).
6. Clicking OK now stored the dereferenced path into prefs.js.

Comment 6

[David :Bienvenu]

so perhaps its the code that persists the file pref that follows the symlink? i.e., nsIPrefBranch.SetComplexValue with an nsIFile?

Comment 7

[David :Bienvenu]

yeah, http://mxr.mozilla.org/comm-central/source/mozilla/modules/libpref/src/nsPrefBranch.cpp#328 uses mWorkingPath, which I think is the symlink resolved. So this would be a core gecko prefs bug.

Comment 8

[:aceman]

Benjanim, Dan, what do you think of this?

David, there is no mWorkingPath in the file you linked...

Comment 9

[Benjamin Smedberg]

IIRC "symlinks" were really meant for shortcut/.lnk files on Windows, and weren't meant to deal with actual symlinks on *nix-style filesystems, which were supposed to be almost transparent.

Tt appears that the only time we resolve the symlink is the call to realpath in nsLocalFile::Normalize. So I think you need to figure out who is calling Normalize() on the home directory at the "first" runtime when we save the prefs.

Comment 11

[:aceman]

Thanks, Benjamin.

There seems to be a Normalize call in our function that fetches the path pref: http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgIncomingServer.cpp#441

I'll see what happens if I remove it.

Comment 12

[:aceman]

We use normalize to convert from the relative path nsIRelativeFilePref to a nsIFile. It usually uses a lot of '../' so we normalize it. But that seems to expand symlinks. Any way around that?

Comment 13

[Magnus Melin [:mkmelin]]

Maybe we can just skip the normalization?

Comment 14

[Ulf Zibis]

The bug should apply accordingly vice versa with REAL symlinks (not shortcuts) and junctions on Windows OS.

Comment 15

[Charlie Gilbert]

Has there been any resolution to this?  I just installed Fedora 20 and the version of Thunderbird included has this problem.

Comment 17

[Wayne Mery (:wsmwk)]

(In reply to Magnus Melin from comment #13)
> Maybe we can just skip the normalization?

What do you think?

Comment 18

[:aceman]

We could also change nsMsgIncomingServer::GetFileValue to have a new argument aRaw so the accound manager can request the raw path (as stored in prefs) and all other callers can get the default normalized path.

However, the normalize call was added intentionally by bug 643642 so we probably can't change really change it unless we make sure otherwise that bug 643642 will not come back.

Comment 19

[Kent James (:rkent)]

Reading bug 643642, it sounds like what we need to do is to figure out where there is a check that the folder file path patches the db file path, and compare the normalized versions there, and not store normalized versions.

Comment 20

[:aceman]

Attachment: patch

(In reply to Kent James (:rkent) from comment #19)
> Reading bug 643642, it sounds like what we need to do is to figure out where
> there is a check that the folder file path patches the db file path, and
> compare the normalized versions there, and not store normalized versions.

I couldn't find the place where we compare some paths with the database.

However I think we can at least prevent corrupting the path in prefs.js within the account manager itself.

See steps in comment 5.

I fix it at 2 places:
1. in the account settings, selecting a Local directory with links immediatelly dereferenced it inside checkDirectoryIsUsable() because calling .normalize() on the passed in object also caused the normalization to be preserved outside the call. So now I make a clone of the nsIFile object and work on that inside the function. So this allows to preserve the links and makes TB save the path in prefs.js unchanged (with links). (This was additional breakage done after this bug was filed.)

2. The other problem was (original in this bug), that having a symlink in the path (local directory) would return it normalized from GetFileValue (thus GetLocalPath). I don't touch that. But When you click OK in the account manager, it saves back all pref values. So it saved the normalized value back into the path pref thus ruining the link even though the user didn't even touch the local directory field. I fix this in SetFileValue by comparing the original path (returned normalized) with the "new" path to be stored (also normalized) and if they are the same I do not save the pref, thus NOT touching the link in the pref.

3. The rest of the changes is removing apparent duplicate code from Get/SetFileValue via using NS_Get/SetPersistentFile. These changes are optional to the bug fix, I just noticed them while reading the functions. I convert them to the pattern used in nsMsgIdentity::Get/SetSignature() .

Please notice the path will still be shown dereferenced in the account manager's local directory field, but it will be kept in prefs.js unchanged (with symlinks), which is what should make the user's use case work.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=697e068ee3cadc1dc2ab8b4783ebf1aa922b883c

If you like the solution I can do the same fix (2.) for the signature file path too.

Comment 21

[Jorg K (CEST = GMT+2)]

Why did you pick two Windows reviewers when this bug is about Linux?

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8866195 [details] [diff] [review]
patch

Review of attachment 8866195 [details] [diff] [review]:
-----------------------------------------------------------------

Looks plausible, I'll test it when the nits are fixed and the question answered.

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +451,5 @@
>  
> +  // GetFileValue normalizes the path so if the "new" path still points to the
> +  // same place, we do not want to touch the original non-normalized path
> +  // (e.g. with symlinks). Bug 760937.
> +  nsCOMPtr<nsIFile> aCurrentFile;

Bad local variable name. "a" implies argument to the function.

@@ +454,5 @@
> +  // (e.g. with symlinks). Bug 760937.
> +  nsCOMPtr<nsIFile> aCurrentFile;
> +  nsresult rv = GetFileValue(aRelPrefName, aAbsPrefName, getter_AddRefs(aCurrentFile));
> +
> +  // The current path may be uset when we are setting it for the first time now.

uset?

::: mailnews/base/util/nsMsgUtils.cpp
@@ +1251,5 @@
> +        rv2 = prefBranch->SetComplexValue(relPrefName, NS_GET_IID(nsIRelativeFilePref), relFilePref);
> +        if (NS_FAILED(rv2)) {
> +            if (NS_SUCCEEDED(rv))
> +                prefBranch->ClearUserPref(relPrefName);
> +        } else

Needs braces.

@@ +1252,5 @@
> +        if (NS_FAILED(rv2)) {
> +            if (NS_SUCCEEDED(rv))
> +                prefBranch->ClearUserPref(relPrefName);
> +        } else
> +          return rv2;

Why did you change the logic here? You return success here and rv is ignored.

Comment 23

[:aceman]

(In reply to Jorg K (GMT+2) from comment #22)
> @@ +1252,5 @@
> > +        if (NS_FAILED(rv2)) {
> > +            if (NS_SUCCEEDED(rv))
> > +                prefBranch->ClearUserPref(relPrefName);
> > +        } else
> > +          return rv2;
> 
> Why did you change the logic here? You return success here and rv is ignored.

rv is returned if rv2 failed.

I wanted to express the idea that if rv2 succeeded, return that. If it failed, return whatever rv returned. We can cope if at least one of the prefs gets set.

Comment 24

[:aceman]

Attachment: patch v1.1

Thanks, fixed the nits.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8867325 [details] [diff] [review]
patch v1.1

Review of attachment 8867325 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/util/nsMsgUtils.cpp
@@ +1253,5 @@
> +            if (NS_SUCCEEDED(rv)) {
> +                prefBranch->ClearUserPref(relPrefName);
> +            }
> +        } else
> +          return rv2;

The 'else' still needs braces ;-)

I still don't like the way this is written. Can you do:
if (NS_SUCCEEDED(rv2) && relFilePref) {
  rv2 = prefBranch->SetComplexValue(...
  if (NS_SUCCEEDED(rv2))
    return NS_OK; // Stored relative path, so we're done.
  if (NS_SUCCEEDED(rv))
    prefBranch->ClearUserPref(relPrefName); // Couldn't store relative path, so clear if we have an absolute one.
}

@@ +1258,3 @@
>      }
>  
> +    NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to write file path pref.");

Assertion? Why not just a warning? And please lose the ".".

Comment 26

[:aceman]

(In reply to Jorg K (GMT+2) from comment #25)
> > +    NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to write file path pref.");
> 
> Assertion? Why not just a warning? And please lose the ".".

I copied this from somewhere. Maybe because if we can't save the account directory, we are hosed :)
So how would I do a warning?

Comment 27

[Jorg K (CEST = GMT+2)]

NS_WARNING()? NS_ASSERTION() and NS_WARNING() only have effect in a debug build.

Comment 28

[Jorg K (CEST = GMT+2)]

On Windows I have a profile at C:\Users\jorgk\AppData\Roaming\Thunderbird\Profiles\testing.testing\. I created a link (yes, the do exist in Windows), like so:
mklink /j d:\desktop\link c:\Users\jorgk\AppData\Roaming\Thunderbird\Profiles\testing.testing
and then changed my profile.ini to do this:

[Profile2]
Name=testing
IsRelative=0
Path=D:\DESKTOP\link
Default=1

Using Daily without the patch I visited various accounts in the Account Settings and checked the Local Directory. All accounts had local directories starting with D:\DESKTOP\link, so nothing was dereferenced here.

However, I noticed that
mail.server.server2.directory was initially (before I started testing with the link)
  C:\Users\jorgk\AppData\Roaming\Thunderbird\Profiles\testing.testing\Mail\Local Folders
and that changed to
  D:\DESKTOP\link\Mail\Local Folders
after clicking OK when visiting "Local Folders" in the Account Settings. That's expected and OK, no?

With the patch applied, going "OK" in the Account Settings does *not* change the preference value any more if it points to effectively the same path. Surprising? Desirable? Inconsequential? Maybe I don't really like this twisted logic that SetFileValue() won't change the value if it points to the previous target. Isn't that a little hacky and might cause problems elsewhere?

Next I'll switch to Linux and see what that does.

Comment 29

[:aceman]

(In reply to Jorg K (GMT+2) from comment #28)
> Using Daily without the patch I visited various accounts in the Account
> Settings and checked the Local Directory. All accounts had local directories
> starting with D:\DESKTOP\link, so nothing was dereferenced here.
> 
> However, I noticed that
> mail.server.server2.directory was initially (before I started testing with
> the link)
>  
> C:\Users\jorgk\AppData\Roaming\Thunderbird\Profiles\testing.
> testing\Mail\Local Folders
> and that changed to
>   D:\DESKTOP\link\Mail\Local Folders
> after clicking OK when visiting "Local Folders" in the Account Settings.
> That's expected and OK, no?

That is expected as in it does happen now without the patch. I consider that strange that the value is touched even if you didn't do anything with it in the Account settings.
It may be this normalization (dereferencing) works differently on Windows. On linux without the patch it would actually work he other way round: D:\DESKTOP\link\Mail\Local Folders would be changed to C:\Users\jorgk\AppData\Roaming\Thunderbird\Profiles\testing. It may be Windows can some the difference thanks to the drive letter.
 
> With the patch applied, going "OK" in the Account Settings does *not* change
> the preference value any more if it points to effectively the same path.
> Surprising? Desirable? Inconsequential?

That is expected. Why should it change when you didn't click the Browse button? Can you try that? If you browse intentionally choose the D:\DESKTOP\link\Mail\Local Folders now and it is not stored in that way, that would be a bug in my patch.

> Maybe I don't really like this
> twisted logic that SetFileValue() won't change the value if it points to the
> previous target. Isn't that a little hacky and might cause problems
> elsewhere?

The idea is, why should the value change in any way, if you haven't touched it (knowingly)?

Comment 30

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #29)
> If you browse intentionally choose the
> D:\DESKTOP\link\Mail\Local Folders now and it is not stored in that way,
> that would be a bug in my patch.
I tried that before, it works. If I browse to the same folder but with/without a link this time, the preference changes. Care to explain how/why, since I thought that SetFileValue() would ignore the change.

Comment 31

[:aceman]

(In reply to Jorg K (GMT+2) from comment #30)
> I tried that before, it works. If I browse to the same folder but
> with/without a link this time, the preference changes. Care to explain
> how/why, since I thought that SetFileValue() would ignore the change.

It compares the original normalized path to the new path non-normalized. So in my tests, the normalized path was absolute path and even if I chose the same path but using a link, the new path was different so it got stored.

So it depends on how your original path gets interpreted. If you just open Account setting, what does Local directory show. It should show the original normalized path. Is it the C: path or the D: one?

Comment 32

[Jorg K (CEST = GMT+2)]

I can't reply to comment #31 since I'm on Linux now.

I create a link:
test -> .thunderbird/test.test/
and my profile reads:

Name=test
IsRelative=0
Path=/home/jorgk/test
Default=1

Looking at the local folders I still see:
/home/jorgk/.thunderbird/test.test/Mail/Local Folders
So that's much different to Windows, it's shown dereferenced. If I navigate to "test/Mail/Local Folders"
mail.server.server2.directory is stored as /home/jorgk/test/Mail/Local Folders
and that sticks although, as I said, the UI always shows the dereferenced value after a restart of TB.

I think the fundamental problem here is that there is too much dereferencing going on. If the profile is defined via a link, why does the link show resolved in the UI? Equally, if the user navigates to a folder via a link, why does the result not show in the UI. The patch seems to be a hack to work around that. But it makes things even more confusing. User chooses X, X is stored, but Y is shown.

As comment #0 says:
Application should always use the symlink. Symlinks are supposed to mask underlying hardware and should not be dereferenced by the application, only by the system at the time of call.

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 8867325 [details] [diff] [review]
patch v1.1

I can see all the work that went into that (and I had work myself to refresh my Linux box), but I'm afraid that comment #32 makes that an f-.

Also, comment #28, 2nd last paragraph makes me feel uneasy.

(In reply to :aceman from comment #29)
> The idea is, why should the value change in any way, if you haven't touched
> it (knowingly)?
Sure, but if the correct value were shown in the UI, you could store it back all you like without causing any damage. The problem is that the *wrong* value is shown, and you jump through hoops to maintain the *right* value that was previously stored.

Comment 34

[:aceman]

Yes exactly, it is a hack. The UI still shows the dereferenced path, but with the patch we at least store and preserve what the user has chosen (including the links). The app internally uses the dereferenced paths. Yes, that is bug but was made intentional by bug 643642. That is what I said from the start.

So the real fix would be to find out why bug 643642 fix was needed. That is what Kent says in comment 19.
Where is the database that stores the normalized paths that must then be compared to the account paths? We see e.g. panacea.dat stores some paths.

Comment 35

[:aceman]

Thanks for playing with it. Some parts of the patch will be needed in all cases.

I can check the other way of fixing it. It needs to audit all callers of GetFileValue and GetLocalPath to see what they do with the path. If one of them needs to use the normalized path, it can do it internally. I can look at that.

But we need to consider that e.g. panacea.dat already contains the normalized paths. If we suddenly start to send it non-normalized paths that will not match, we will see what problems that will cause. Maybe it will just start to store new entries with new paths and everything will be fine after the first run with a TB with this change. On the other hand, some of the data in panacea seems valuable not to lose.

Comment 36

[Jorg K (CEST = GMT+2)]

Now back on Windows, coming back to comment #28:
===
However, I noticed that
mail.server.server2.directory was initially (before I started testing with the link)
  C:\Users\jorgk\AppData\Roaming\Thunderbird\Profiles\testing.testing\Mail\Local Folders
and that changed to
  D:\DESKTOP\link\Mail\Local Folders
after clicking OK when visiting "Local Folders" in the Account Settings. That's expected and OK, no?

With the patch applied, going "OK" in the Account Settings does *not* change the preference value any more ...
===

I got confused here with mail.server.server2.directory and mail.server.server2.directory-rel.

Changing the profile from a relative path "Profiles/testing.testing" to an absolute path via a link D:\DESKTOP\link that points to the very same folder will leave mail.server.server2.directory-rel untouched with this value "[ProfD]Mail/Local Folders". Going OK on the Account Settings without your patch will always refresh the absolute path in mail.server.server2.directory, with the patch, since the relative directory doesn't change at all, mail.server.server2.directory doesn't get updated as observed (which isn't really good since it's out of step).

And comment #30:
"Care to explain how/why, since I thought that SetFileValue() would ignore the change."
Well, a little debugging shows:
=== Current file D:\DESKTOP\link\Mail\Local Folders
=== New file C:\Users\jorgk\AppData\Roaming\Thunderbird\Profiles\testing.testing\Mail\Local Folders
=== different path
although effectively they are the same.

I haven't tried this on Linux, but you said in comment #31:
"It compares the original normalized path to the new path non-normalized" which I find highly confusing. I don't understand why Linux would find that stored "/home/jorgk/test/Mail/Local Folders" (test being a link to .thunderbird/test.test/) and displayed "/home/jorgk/.thunderbird/test.test/Mail/Local Folders" would be the same when OK is clicked and how navigating to "/home/jorgk/.thunderbird/test.test/Mail/Local Folders" would be different and change the stored value.

Let me attach my debug patch and then try this on Linux to close the loop.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: 760937-debug.patch

Comment 38

[Jorg K (CEST = GMT+2)]

Linux going OK shows:
=== Current file /home/jorgk/.thunderbird/test.test/Mail/Local Folders
=== New file /home/jorgk/.thunderbird/test.test/Mail/Local Folders
=== same path
when the UI showed: /home/jorgk/.thunderbird/test.test/Mail/Local Folders
and mail.server.server2.directory had "/home/jorgk/test/Mail/Local Folders" (via link).

Now, navigating to /home/jorgk/.thunderbird/test.test/Mail/Local Folders also gets the same path, so once "/home/jorgk/test/Mail/Local Folders" (via link) is set, you can never remove the link any more.

Did I say I was uneasy with some logic that ignored user input :-( I think this approach doesn't fly.

Comment 39

[Jorg K (CEST = GMT+2)]

Reading back through this bug and bug 643642 I get the impression that "normalising" is the wrong thing to do, especially since it resolves links on Linux. The motivation for bug 643642 is rather weak, "If a user edits profile.ini ...". Sure, if they edit the binaries or omni.ja with a text or hex editor, then things will fall apart, too.

If anything, we should do a "sanitise" or "standardise", that means, remove unneeded "./" at the front and similar.

Compare the Windows version which does that
http://searchfox.org/mozilla-central/rev/06b337ab22d40e12c4cde8040b23147ea636cc6e/xpcom/io/nsLocalFileWin.cpp#1538
to the Linux version that resolves links via 'realpath':
http://searchfox.org/mozilla-central/rev/06b337ab22d40e12c4cde8040b23147ea636cc6e/xpcom/io/nsLocalFileUnix.cpp#550

Comment 40

[:aceman]

(In reply to Jorg K (GMT+2) from comment #38)
> Now, navigating to /home/jorgk/.thunderbird/test.test/Mail/Local Folders
> also gets the same path, so once "/home/jorgk/test/Mail/Local Folders" (via
> link) is set, you can never remove the link any more.

Yes, if you have the link stored, you can't store the full path now that points to the same place. That is a real problem with my patch. But this case may be less often than converting to a link.

Comment 41

[:aceman]

(In reply to Jorg K (GMT+2) from comment #36)
> Changing the profile from a relative path "Profiles/testing.testing" to an
> absolute path via a link D:\DESKTOP\link that points to the very same folder
> will leave mail.server.server2.directory-rel untouched with this value
> "[ProfD]Mail/Local Folders". Going OK on the Account Settings without your
> patch will always refresh the absolute path in
> mail.server.server2.directory, with the patch, since the relative directory
> doesn't change at all, mail.server.server2.directory doesn't get updated as
> observed (which isn't really good since it's out of step).

We ignore the .directory pref if .directory-rel exists. So it is not a problem if it contains incorrect value. But yes, to lessen user confusion when reading the file, we could sync them. That could be fixed in my patch.

Comment 42

[:aceman]

(In reply to Jorg K (GMT+2) from comment #39)
> If anything, we should do a "sanitise" or "standardise", that means, remove
> unneeded "./" at the front and similar.

Yes, this would be enough, but it seems on Linux normalize does much more than that, which is unwanted sideeffect. But we do not have a better function.

Comment 43

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #42)
> Yes, this would be enough, but it seems on Linux normalize does much more
> than that, which is unwanted sideeffect. But we do not have a better
> function.
We could write one. As far as I can tell, it works fine on Windows right now, so instead of calling Normalize() (bug 643642, https://hg.mozilla.org/comm-central/rev/210f183920f2) we call our own.

Maybe we can copy something from GNU's "realpath" http://ftp.gnu.org/gnu/coreutils/coreutils-8.27.tar.xz (src/realpath.c and lib/canonicalize.c) since that has a --no-symlinks swich, so it will leave links alone.

Comment 44

[Kent James (:rkent)]

Comment on attachment 8867325 [details] [diff] [review]
patch v1.1

I think that Magnus is a better reviewer here as he is more familiar with Linux issues.

Comment 45

[Magnus Melin [:mkmelin]]

Comment on attachment 8867325 [details] [diff] [review]
patch v1.1

Review of attachment 8867325 [details] [diff] [review]:
-----------------------------------------------------------------

On linux it seems to do what it should, preserving the (unresolved) symlink value in the UI.

Comment 46

[JuanNM]

This bug is "open" but seemed like it was on good path to be fixed; any dev can comment on whether this was the case in the end?

Comment 47

[Ulf Zibis]

I still see this issue on Linux, when adding a new account or creating a new profile, see: bug 944949.
The workaround is to manually correct the paths in prefs.js and delete panacea.dat before a new start of TB.