1136620 - -profile option does not create a new directory on linux if the directory does not exist

Status: NEW

(Toolkit :: Startup and Profile System, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: realpath_fix.patch

From realpath's man:

   GNU extensions
       If the call fails with either EACCES or ENOENT and resolved_path is not NULL, then the prefix of path that is not readable or does not exist is returned in resolved_path.

<snip>
The
       resolved_path == NULL feature, not standardized in POSIX.1-2001, but standardized in POSIX.1-2008, allows this design problem to be avoided.


So we can use resolved_path if errno is ENOENT.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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

Comment 2

[Benjamin Smedberg]

Comment on attachment 8569071 [details] [diff] [review]
realpath_fix.patch

I'm not convinced that this is the behavior we want. I expected Firefox to fail to launch (return a nonzero exit code) if we use -profile with a path that doesn't exist. Unless there is a compelling reason to auto-create the directory, I'd prefer to make that explicitly illegal.

Comment 3

[Benjamin Smedberg]

(And this has nothing to do with XPCOM)

Comment 4

[Till Schneidereit [:till]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Comment on attachment 8569071 [details] [diff] [review]
> realpath_fix.patch
> 
> I'm not convinced that this is the behavior we want. I expected Firefox to
> fail to launch (return a nonzero exit code) if we use -profile with a path
> that doesn't exist. Unless there is a compelling reason to auto-create the
> directory, I'd prefer to make that explicitly illegal.

FWIW, on Windows and OS X, we do what the proposed patch would add to Linux, too. I know that quite a few people use it to create throwaway-profiles like so: firefox -profile /tmp/somerandomname.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Till Schneidereit [:till] from comment #4)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> > Comment on attachment 8569071 [details] [diff] [review]
> > realpath_fix.patch
> > 
> > I'm not convinced that this is the behavior we want. I expected Firefox to
> > fail to launch (return a nonzero exit code) if we use -profile with a path
> > that doesn't exist. Unless there is a compelling reason to auto-create the
> > directory, I'd prefer to make that explicitly illegal.
> 
> FWIW, on Windows and OS X, we do what the proposed patch would add to Linux,
> too. I know that quite a few people use it to create throwaway-profiles like
> so: firefox -profile /tmp/somerandomname.

Also worth noting that we have stuff in-tree that depends on this:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#940

Comment 6

[Hiroyuki Ikezoe (:hiro)]

So, which behavior should firefox take in case of -profile option?

a) create the directory if not exist
b) just abort

Current implementation is inconsistency between Linux and others. I will provide a patch to fix the inconsistency.

I know some companies use -profile option for daily use on Windows, and they rely on a). If firefox take b), they will need a script which creates directory on first launch of firefox.

Comment 7

[Benjamin Smedberg]

Comment on attachment 8569071 [details] [diff] [review]
realpath_fix.patch

ok, given that mach depends on this behavior, I guess we should support using -profile for a directory that doesn't yet exist.

What happens if the path is relative? I feel like this will still fail:

firefox -profile ../foo-notpresent

Because realpath isn't making the path absolute. The realpath docs say: "If the call fails with either EACCES or ENOENT and resolved_path is not NULL, then the prefix of path that is not readable or does not exist is returned in resolved_path. " Which is either confusing or will produce incorrect results in this case.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

That works.

From the man of realpath.

realpath()  expands  all  symbolic  links and resolves references to /./, /../ and extra '/' characters in the null-terminated string named by path to produce a canonicalized absolute pathname.

Comment 9

[Benjamin Smedberg]

The man page is talking about what happens when realpath succeeds. But this patch deals with the case where realpath fails (with ENOENT). I was quoting the manpage for that case in comment 7.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I am sorry for my misunderstanding. Actually I did try relative path against non-existent directory with atachment #8569071 when I commented comment #8. And now I tried it again for confirmation, it worked!

Comment 11

[Benjamin Smedberg]

I still don't understand *why* it works. Can you explain what happens to the buffers in question when we call realpath() on a path that doesn't exist and it returns ENOENT? Is that behavior well-defined or accidental? Is it a POSIX behavior or Linux-only, and will this cause problems on the BSDs or even perhaps on mac?

I feel like the man page is saying that the behavior is not what we actually want, which is why I'm concerned about compatibility and correctness here.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8569071 [details] [diff] [review]
realpath_fix.patch

Ah, the behavior is GNU extension, will not work on BSD.

Comment 13

[YUKI "Piro" Hiroshi]

Any progress?