1364984 - GetSpecialSystemDirectory fails in case OS_TemporaryDirectory on macOS Sierra if user home directory is on NFS volume

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Colin Dean]

Attachment: Objective-C to test determining temporary directory path on macOS

Method GetSpecialSystemDirectory() in xpcom/io/SpecialSystemDirectory.cpp, case OS_TemporaryDirectory uses Cocoa function FSFindFolder(), which was deprecated by Apple in OS X 10.9.

This fails on macOS Sierra (tested with 10.12.3 and 10.12.4) if user's home directory is on an NFS-mounted volume.  This can be demonstrated in a standalone non-GUI test program - source with detailed comments attached.  It worked in OS X 10.11 and earlier.

One show-stopping effect of this is in Thunderbird, where any attempt to send or save a composed email fails (because there is no directory in which to create the temporary file containing the email).  Demonstrated in 52.1.0 and in Trunk.  This is the underlying cause of Thunderbird Bug 1363190.

A more appropriate function to use, for all versions of OS X / macOS supported by Firefox, Thundrbird, etc, would be Apple's Foundation Framework NSTemporaryDirectory(), which works regardless of whether the user's home directory volume is local or remote, as demonstrated in test program.

I will post a simple patch that fixes this.

Comment 1

[Colin Dean]

Attachment: Patch to replace use of FSFindFolder() by NSTemporaryDirectory()

Method added to CocoaFileUtils.mm bridges NSTemporaryDirectory() in obj-c as a CFURLRef for c++.

Method added to SpecialSystemDirectory.cpp makes use of fact that we can replace use of InitWithFSRef() by existing InitWithCFURL().

Tested and confirmed working in Thunderbird (Trunk and 52.1.0), with both local and NFS volumes, for 4 different users of desktops running macOS 10.12.4, with NFS server running CentOS 6.9.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8867809 [details] [diff] [review]
Patch to replace use of FSFindFolder() by NSTemporaryDirectory()

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

Thanks for the patch!  I'm not a Mac expert, but a couple of points below.

Also, I think you generated your diff by doing something like:

$ cp path/to/CocoaFileUtils.h path/to/CocoaFileUtils.h.orig
[...similar steps for the other files...]
[...edit files...]
$ diff -u ...some arguments...

which works great for letting us see what you're trying to do, but not so great for us trying to automatically apply your patch to our local trees or committing it.  Please use mercurial for managing commits and patches; you can find information on using mercurial:

https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html

Also, for future patches, please set the review flag on the patch.

::: SpecialSystemDirectory.cpp.orig
@@ +825,5 @@
> +  CFURLRef url;
> +  nsresult rv = NS_ERROR_FAILURE;
> +
> +  url = CocoaFileUtils::GetTemporaryFolderCFURLRef();
> +  if (url != NULL) {

The convention Firefox uses is to return early from functions whenever possible.  Please write this as:

CFURLRef url = CocoaFileUtils::GetTemporaryFolderCFURLRef();
if (url == nullptr) {
  return NS_ERROR_FAILURE;
}
[...continue on...]

This reduces indentation and also makes it easier to see the expected path of code through the function.

Also, `url` will need to be CFRelease'd before the function returns.

@@ +829,5 @@
> +  if (url != NULL) {
> +    NS_NewLocalFile(EmptyString(), true, aLocalFile);
> +    nsCOMPtr<nsILocalFileMac> localMacFile(do_QueryInterface(*aLocalFile));
> +    if (localMacFile) {
> +      rv = localMacFile->InitWithCFURL(url);

We should be able to write all this as:

nsCOMPtr<nsILocalFileMac> localFile;
nsresult rv = NS_NewLocalFileWithCFURL(..., getter_AddRefs(localFile));
if (NS_FAILED(rv)) {
  ::CFRelease(url);
  return rv;
}

localFile.forget(aLocalFile);
::CFRelease(url);

return NS_OK;

Comment 3

[Colin Dean]

Thanks Nathan, I agree with everything you wrote.

As for coding conventions, I just chose to layout GetOSXTemporaryFolder() exactly like the existing GetOSXFolderType() above it as it makes it visually very easy to compare the two methods.  It wouldn't be my preferred style, I would instinctively have laid it out as you suggested if I were starting from scratch.

Sadly, I'm not able to spend the time I might like contributing to Firefox, as being CTO of a non-profit keeps me occupied, but for us Thunderbird is the only credible cross-platform (Linux + Mac) email client that handles custom from addresses, multiple signatures, calendar integration and more, and actually works, so I'm very happy to donate a fix.  This was my first hands-on experience of the Mozilla code base and tools, despite having been programming since the late 70's, and it was great to be able to build, identify and fix in just a few hours. 

Colin

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: use non-deprecated APIs for finding the temporary directory on OS X

FSFindFolder is deprecated as of 10.9 and apparently continues to
work...except for the case of finding the temporary directory when the
user's home directory lives on an NFS mount (!).  Using
NSTemporaryDirectory does The Right Thing here, so let's use that instead.

Colin, I used your email address from the example Obj-C program as the author
here; please let me know if that's OK or if there's a different email address
you'd prefer I use.

It's not clear to me whether we should handle this in GetOSXFolderType as a
special case, or whether the current setup is OK.  Or perhaps GetOSXFolderType
is completely bogus now, and we should completely redo how all of this is handled?

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8870453 [details] [diff] [review]
use non-deprecated APIs for finding the temporary directory on OS X

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

This seems to be a strict improvement over the previous code, so let's do this for now. I'll leave the question about GetOSXFolderType to Stephen.

Comment 6

[Colin Dean]

Attachment: Fixes 2 problems in previous patch

Nathan's patch doesn't compile because of 2 problems it creates with SpecialSystemDirectory.cpp:

1. Argument in GetOSXTemporaryFolder() definition should be aLocalFile, not aFile.
2. Inclusion of CocoaFileUtils.h needs to be inside XP_UNIX block, not as an alternative to it, as XP_UNIX is defined on macOS.

I've built Thunderbird 52.1.0 on macOS with my updated patch and it runs correctly.

Comment 7

[Colin Dean]

I'm perfectly happy to be identified as "Colin Dean <[e-mail]>" as author of patch.  I've updated my Buzgilla profile to match this.

Comment 8

[Stephen A Pohl [:spohl]]

Comment on attachment 8870453 [details] [diff] [review]
use non-deprecated APIs for finding the temporary directory on OS X

(In reply to Nathan Froyd [:froydnj] from comment #4)
> Created attachment 8870453 [details] [diff] [review]
> It's not clear to me whether we should handle this in GetOSXFolderType as a
> special case, or whether the current setup is OK.  Or perhaps
> GetOSXFolderType
> is completely bogus now, and we should completely redo how all of this is
> handled?

Based on my reading (which included [1]), I believe we will have to slowly move away from this kind of setup to find the appropriate folders. Rather than having one routine (FSFindFolder) to locate all of them, we will have to call several different ones (NSTemporaryDirectory, NSHomeDirectory, NSFileManager's URLForDirectory). I would prefer it if we could contain this logic in GetOSXFolderType, rather than introducing several different new functions like the GetOSXTemporaryFolder function proposed here. We should solve the larger problem of modernizing this code in a separate bug, and expand GetOSXFolderType as necessary.

[1] https://stackoverflow.com/questions/19404239/locating-mac-os-x-folders-using-urlfordirectory-instead-of-fsfindfolder

Comment 9

[Colin Dean]

I agree with "slowly move away". Probably worth waiting for the successor to macOS Sierra (presumably in beta after WWDC).  Apple might have deprecated/removed/broken more things Mozilla code base relies on.

Meanwhile, as submitter of this report (not a Mozilla maintainer) I'm responsible for network of Macs using NFS where current stock build of Thunderbird is completely non-functional.  So in the short term, if someone could adopt my attachment 8870532 [details] [diff] [review] from 2 days ago and get it committed, I'd be really grateful.  Thanks.

Comment 10

[Colin Dean]

I've confirmed that Apple's FSFindFolder() and NSTemporaryDirectory() behave exactly the same in recently-released macOS Sierra 10.12.5 as they did in 10.12.3 and 10.12.4.  Proposed patch is still necessary and continues to work.

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: Patch

Colin, I've addressed my feedback in your patch. Could you please confirm that this patch still works as expected for you? If so, we can ask Markus for final review and get this committed. Thank you!

Comment 12

[Colin Dean]

Hi Stephen. This patches, builds and runs correctly using Thunderbird 52.1.0 source on MacOS Sierra 10.12.5 with Xcode 8.3.2.  Thank you!

Comment 13

[Stephen A Pohl [:spohl]]

Comment on attachment 8872641 [details] [diff] [review]
Patch

(In reply to Colin Dean from comment #12)
> Hi Stephen. This patches, builds and runs correctly using Thunderbird 52.1.0
> source on MacOS Sierra 10.12.5 with Xcode 8.3.2.  Thank you!

Perfect, thank you!

Comment 14

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60752371afbb6a9e0631c55ea09a05c0d767d04b
Bug 1364984 - use non-deprecated API for finding the temporary directory on OS X; r=mstange

Comment 15

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/130dc8355e27d6161af44fb2e0e65212f75bd730
Backout 60752371afbb (bug 1364984) for causing bc failures. r=me

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This also upset (at least) OSX XPCShell tests like https://treeherder.mozilla.org/logviewer.html#?job_id=103828572&repo=mozilla-inbound

Comment 17

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f80d811b7e71521a05bad4b2c31021e3dec07dd

Comment 18

[Stephen A Pohl [:spohl]]

Attachment: Changes for sandbox & python tests

:haik, asking for you review of the change that you suggested in dom/ipc/ContentChild.cpp. This fixes security/sandbox/test/browser_content_sandbox_fs.js for me locally. Kicked off a try build in comment 17 to confirm.

:aswan, asking for your review of the change in toolkit/modules/subprocess/test/xpcshell/test_subprocess.js. Python seems to return a path starting with "/private/var" from pwd, but we now expect a path starting with "/var", which is a symlink to "/private/var".

Comment 19

[Haik Aftandilian [:haik]]

Comment on attachment 8874928 [details] [diff] [review]
Changes for sandbox & python tests

r+ for the dom/ipc/ContentChild.cpp changes.

Stephen, could you make sure you test that printing and printing to PDF still work on Mac with the change? Printing depends on being able to write to the ContentTempDirectory, but it's not covered by automated tests.

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8874928 [details] [diff] [review]
Changes for sandbox & python tests

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

Can you readlink() tmpDir instead of hard-coding platform-specific logic about the name /private ?

Comment 21

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84eae5f289074288b5dfc9d00c499af754e2d28c

Comment 22

[Stephen A Pohl [:spohl]]

Attachment: Changes for sandbox & python tests

(In reply to Andrew Swan [:aswan] from comment #20)
> Comment on attachment 8874928 [details] [diff] [review]
> Changes for sandbox & python tests
> 
> Review of attachment 8874928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you readlink() tmpDir instead of hard-coding platform-specific logic
> about the name /private ?

I changed this to use os.path.realpath().

I also noticed that xpcom/tests/unit/test_file_equality.js was failing in the previous try run. This was because the test assumed that tmp1 was normalized. I added a fix to this patch. :aswan, are you able to review this or forward to the right person? Thank you!

Comment 23

[Stephen A Pohl [:spohl]]

(In reply to Haik Aftandilian [:haik] from comment #19)
> Stephen, could you make sure you test that printing and printing to PDF
> still work on Mac with the change? Printing depends on being able to write
> to the ContentTempDirectory, but it's not covered by automated tests.

I will test this tomorrow (technically, today). Thank you for the heads up!

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8875110 [details] [diff] [review]
Changes for sandbox & python tests

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

getting close!

::: toolkit/modules/subprocess/test/xpcshell/test_subprocess.js
@@ +554,5 @@
> +    return realpathOutput;
> +  }
> +
> +  let procDir = await OS.File.getCurrentDirectory();
> +  let tmpDir = await realpath(OS.Constants.Path.tmpDir);

I think that calling out to python for this is unnecessary.  If you create an nsIFile with this path, you can use the .target property to get the symlink target.

::: xpcom/tests/unit/test_file_equality.js
@@ +24,5 @@
>    do_check_true(exists);
>    if (!exists)
>      return;
>  
> +  // normalize the initial path

Please explain why this is necessary in the comment

Comment 25

[Stephen A Pohl [:spohl]]

Attachment: Changes for sandbox & python tests

(In reply to Andrew Swan [:aswan] from comment #24)
> Comment on attachment 8875110 [details] [diff] [review]
> Changes for sandbox & python tests
> 
> Review of attachment 8875110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> getting close!
> 
> ::: toolkit/modules/subprocess/test/xpcshell/test_subprocess.js
> @@ +554,5 @@
> > +    return realpathOutput;
> > +  }
> > +
> > +  let procDir = await OS.File.getCurrentDirectory();
> > +  let tmpDir = await realpath(OS.Constants.Path.tmpDir);
> 
> I think that calling out to python for this is unnecessary.  If you create
> an nsIFile with this path, you can use the .target property to get the
> symlink target.

I couldn't get this to work because it keeps throwing NS_ERROR_FILE_INVALID_PATH. Also, the wording for this exception in the documentation[1] is a bit concerning ("Indicates that this nsIFile does not reference a symbolic links."), as it seems to insinuate that it is expected for this exception to be thrown if the path is not a symlink, which would be the case on all platforms but macOS. Do we know for sure that nsIFile target still works? I couldn't find any tests that exercise this and we don't seem to rely on it in our code base.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile#Attributes

> ::: xpcom/tests/unit/test_file_equality.js
> @@ +24,5 @@
> >    do_check_true(exists);
> >    if (!exists)
> >      return;
> >  
> > +  // normalize the initial path
> 
> Please explain why this is necessary in the comment

Fixed.

Comment 26

[Haik Aftandilian [:haik]]

(In reply to Stephen A Pohl [:spohl] from comment #25)
> Created attachment 8875287 [details] [diff] [review]
> Changes for sandbox & python tests
> 
> (In reply to Andrew Swan [:aswan] from comment #24)
> > Comment on attachment 8875110 [details] [diff] [review]
> > Changes for sandbox & python tests
> > 
> > Review of attachment 8875110 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > getting close!
> > 
> > ::: toolkit/modules/subprocess/test/xpcshell/test_subprocess.js
> > @@ +554,5 @@
> > > +    return realpathOutput;
> > > +  }
> > > +
> > > +  let procDir = await OS.File.getCurrentDirectory();
> > > +  let tmpDir = await realpath(OS.Constants.Path.tmpDir);
> > 
> > I think that calling out to python for this is unnecessary.  If you create
> > an nsIFile with this path, you can use the .target property to get the
> > symlink target.
> 
> I couldn't get this to work because it keeps throwing
> NS_ERROR_FILE_INVALID_PATH. Also, the wording for this exception in the
> documentation[1] is a bit concerning ("Indicates that this nsIFile does not
> reference a symbolic links."), as it seems to insinuate that it is expected
> for this exception to be thrown if the path is not a symlink, which would be
> the case on all platforms but macOS. Do we know for sure that nsIFile target
> still works? I couldn't find any tests that exercise this and we don't seem
> to rely on it in our code base.

What about using nsIFile.Normalize()? It resolves symlinks on UNIXy platforms.

Comment 27

[Andrew Swan [:aswan]]

Comment on attachment 8875287 [details] [diff] [review]
Changes for sandbox & python tests

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

If normalize() resolves symlinks, please just use that in the subprocess test (bonus points for actually documenting that on mdn or in the idl)
As for the comment in test_file_equality.js, I was looking more for an explanation of why the path needs to be normalized (e.g., "the test logic below assumes we're starting with a normalized path but the default location on macos is a symbolic link so resolve it before starting")

Comment 28

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d51b9e76eff849735e80e2c47037b013612f643

Comment 29

[Stephen A Pohl [:spohl]]

Attachment: Changes for sandbox & python tests

Try run in comment 28.

(In reply to Andrew Swan [:aswan] from comment #27)
> If normalize() resolves symlinks, please just use that in the subprocess
> test (bonus points for actually documenting that on mdn or in the idl)

I've documented this on mdn, as suggested.

Comment 30

[Andrew Swan [:aswan]]

Comment on attachment 8875434 [details] [diff] [review]
Changes for sandbox & python tests

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

Thanks!

Comment 31

[Stephen A Pohl [:spohl]]

(In reply to Stephen A Pohl [:spohl] from comment #23)
> (In reply to Haik Aftandilian [:haik] from comment #19)
> > Stephen, could you make sure you test that printing and printing to PDF
> > still work on Mac with the change? Printing depends on being able to write
> > to the ContentTempDirectory, but it's not covered by automated tests.
> 
> I will test this tomorrow (technically, today). Thank you for the heads up!

I've just confirmed that printing and printing to PDF still works.

Comment 32

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a3691625489b1f21fca5aa93cb8eee626068a19
Bug 1364984 - use non-deprecated API for finding the temporary directory on OS X; r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/76dd2108b4ae49d518e70a97e5070894685af41f
Bug 1364984 - Ensure that our sandbox and tests can handle temp directory paths using symlinks on macOS. r=haik,aswan

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a3691625489
https://hg.mozilla.org/mozilla-central/rev/76dd2108b4ae

Comment 35

[Jorg K (CEST = GMT+2)]

Stephen, any chance we can get this uplifted to mozilla-esr52 since it affects TB 52 ESR.

If not, which patch do I need to merge onto the TB release branch on mozilla-esr52? The first one or both?

Comment 36

[Colin Dean]

We have been running our own build of TB 52.1 patched as above on macOS 10.12.5 without problems for several weeks now.  If this could be retro-fitted to 52 ESR release it would save us having to maintain our own builds of 52.2 etc.  We don't want to move to 55, because we're also using CentOS 6.9, where 52 is still the supplied version of TB and FF.

Comment 37

[Stephen A Pohl [:spohl]]

Comment on attachment 8872641 [details] [diff] [review]
Patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This affects users of macOS 10.12+.
User impact if declined: From comment 0: "One show-stopping effect of this is in Thunderbird, where any attempt to send or save a composed email fails (because there is no directory in which to create the temporary file containing the email)."
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Minimal. Patch has been riding the 55 train with no reported regressions.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 38

[Stephen A Pohl [:spohl]]

Comment on attachment 8875434 [details] [diff] [review]
Changes for sandbox & python tests

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This affects users of macOS 10.12+.
User impact if declined: From comment 0: "One show-stopping effect of this is in Thunderbird, where any attempt to send or save a composed email fails (because there is no directory in which to create the temporary file containing the email)."
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Minimal. Patch has been riding the 55 train with no reported regressions.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 39

[Gerry Chang [:gchang]]

Comment on attachment 8872641 [details] [diff] [review]
Patch

The issue affects TB 52 ESR. Let's uplift it to ESR52.3.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/3a9410860139
https://hg.mozilla.org/releases/mozilla-esr52/rev/a0899a3dcd7e