322865 - isExecutable method of nsILocalFile reports false for executable files on OSX

Status: NEW

(Core :: XPCOM, defect)

Description

[Dave Liebreich [:davel]]

fl_class=Components.classes["@mozilla.org/file/local;1"];
ln=fl_class.createInstance(Components.interfaces.nsILocalFile);
ln.initWithPath("/bin/ls");
js> ln.isExecutable(); // this reports false ?!

isExecutable reports true for directories with the application flag set (an application bundle), but not for simple executable programs.

This does not match the unix behavior, where the above would report true.

Comment 1

[Dave Liebreich [:davel]]

additionally, isSymlink() reports true for apple aliases, but false for unix symlinks.

Comment 2

[Josh Aas]

I'll look into this. Dave - dougt was a good guess at an owner for this bug, but anything in xpcom that is specific to Mac OS X should probably go through me or Mark Mentovai first.

Comment 3

[Doug Turner (:dougt)]

I asked him to assign this to me first. :-)

Comment 4

[Josh Aas]

My bad then - I just try to save large component owners some trouble when the problem is specific to Mac OS X.

Comment 5

[Josh Aas]

Doug - can we remove Mac OS 8/9 support from nsLocalFileMac? It makes the code messy and causes us to do extra work. I'll probably clean up the file first to make further fixes more clear. Also, the mixed indentation in that file makes it hard to read.

Comment 6

[Josh Aas]

Actually, we should probably just remove nsLocalFileMac. We don't ever use it.

Comment 7

[Josh Aas]

Attachment: possible fix

Something like this will probably work. This patch compiles, and Firefox runs with it, but I haven't tested the actual functionality yet. Because of that, I'm not asking for reviews. I'll test it soon if nobody else does.

Comment 8

[Josh Aas]

Just a reminder - fixing this could have Ts consequences. Another thing to test in addition to the functionality.

Comment 9

[Dave Liebreich [:davel]]

Problem: isExecutable on unix reports true for directories where the execute permission bits are set.

Now isExecutable on OSX reports the same.  But all app bundles are directories (and most dirs have +x)

If the intent of isExecutable is to return true if the file is something that can be run, then it is broken on unix and OSX (with this patch).

Sigh.  I'm not sure what should be done for this case . . .

I did do some basic tests

isSymlink:
regular file: false
mac alias: true
symlink: true

isExecutable:
regular file not executable: false
regular file, various x bits set: true
dir, -x: false
dir, +x: true
file, app bundle bit set, -x: true

Comment 10

[Josh Aas]

So what you're saying is broken is broken with or without this patch right? If I understand what you're saying, it is an easy fix.

Comment 11

[Dave Liebreich [:davel]]

First, your fix for nsLocalFile::isSymlink() on OSX looks great.
 
I'm trying to understand the bigger picture.  I don't understand windows (yet), so the following is limited to unix and OSX.  And I'm ignoring FollowLinks.

On Unix, there is one type of thing that can be run - an executable file.  On OSX, there are two types of things that can be run - an executable file, and something (directory or file) with the application flag set.

What is the purpose of nsLocalFile::isExecutable()?

1. identify executable files (files that can be passed to execve())?

2. identify stuff than can be run (executable files + applications)?

3. identify entities in the filesystem that have one or more execute permission bits set (files or directories)?

I'm thinking it is #2.

Currently (before applying your patch), unix implements 3, and OSX implements 4 (identify applications only).

Your patch changes OSX to implement 2|3.

Now is where I start to get lost in the weeds.  nsProcess::Init(nsIFile) fails if the nsIFile->isFile() is false, so you can't seem to get to the point of calling nsProcess::Run() on a directory.  That seems to make the fact that unix implements 3 (and not 1 or 2) less of an issue.

But that seems to imply that you can't launch an app bundle that is a directory on OSX using nsProcess::Run().  There's special OSX code in Run() to call LaunchApplication if the arg count is 0 . . . does LaunchApplication fall back to execve() on OSX?

Either I'm missing something, or apps and extensions built for OSX don't use nsProcess::Run() and don't use nsLocalFile::isExecutable() the way they are used on unix.

It's late and my brain is fading - I'll talk with dougt tomorrow in the office if this is still confusing.

Comment 12

[Josh Aas]

Attachment: more scratchings

This patch is the same as the last but will not mark directories with the x bit set as executable on UNIX or Mac OS X unless they are application bundles on Mac OS X.

I'll have to put some more thought into the nsProcess thing. Somebody was asking me about that a few months ago, there might be a bug filed somewhere.

Comment 13

[Darin Fisher]

I think we should be careful about changing the meaning of nsIFile::isExecutable.  That is a frozen API, and if we've returned true for directories in the past, then we may need to continue to do so.  At the least, we should document the API better.

Any changes to the API have to be valuable enough to overcome the fact that we said the API wouldn't change.

Comment 14

[Dave Liebreich [:davel]]

after thinking about this for the past week, I believe the first version of the patch is the one that should go in for now.  It does not allow nsProcess to work with application bundles on OSX, but that is the same behaviour as current.

and it leaves unix and win and mac (os9) alone.

Comment 15

[Phil Ringnalda (:philor)]

Some sort of fix that can land on the branch pre-Firefox 2.0 would be nice: between this and bug 307463, the external view-source editor from bug 172817 fails on OS X either way: Foo.app/Contents/MacOS/foo on isExecutable(), Foo.app on nsProcess::init.

Comment 16

[Phil Ringnalda (:philor)]

*** Bug 301539 has been marked as a duplicate of this bug. ***

Comment 17

[Christian Höltje]

FYI: This is currently very broken.

Without the Mac OS X version working, then you might as well not use isExecutable() at all!

If you really want to use isExecutable() a work around for the moment, as crapy as this is....

var is_executable = /Mac/.test(navigator.platform) || nsifile.isExecutable();

Ciao!

Comment 18

[Christian Höltje]

A better OS detect is at:
http://developer.mozilla.org/en/docs/Code_snippets:Miscellaneous#Operating_system_detection

Ciao!

Comment 19

[Jelte Jansen]

Indeed, this is very broken, not having this function return true on executable binary files makes it unusable. The problem is exacerbated by the fact that init() doesn't seem to error (it just does nothing and return 0) so there's no way to reliably check whether you have got an executable file present, without above os-checking hack...

There was a message above saying the API documentation should explain the function better. I'd say add that if you want a portable app, you shouldn't use this at all.

Comment 20

[Georg Maaß]

Who abuses this to check whether a directory has the x flag set? Where does a Windows directory have an x flag? This is an interface to abstract directories and files from the platform specific representations. In this platform independent abstraction there is no x flag for directories.

On the Mac a file is executable if the files x flag is set. An application, that is not a flat file is executable, when it is a directory with name ending with .app and a Info.plist and PkgInfo inside and some other stuff like MacOS and Resources folder inside. If all that stuff is present and fits together it is executable.

If a directory does not match this criterias it is not an executable application but simply a directory. The x flag of a directory that does not represent the root of an excutable application is out of interest, because it is only a directory and therefore never executable. The meaning of the x flag of directories is not executable but an indicator whether it is allowed to browse into or not. So a directory, that is no root of an application should allways return false as value.

If we need platform dependent tools to get the flags or to get file attributes like known in NTFS then we need an additional interface or set of interfaces for that other purpose. But this interface should not be abused for that.

Comment 21

[Davide Ficano]

If somebody is interested in a reasonable (IMHO) workaround take a look at my ViewSourceWith code.

http://dafizilla.svn.sourceforge.net/viewvc/dafizilla/viewsourcewith/src/main/content/viewsourcewith/common.js?revision=1&view=markup#l_85

The code checks if passed filename ends with .app and then opens Info.plist xml file and extracts the CFBundleExecutable tag value.
The CFBundleExecutable contains the "real" exe file name

You should take a look also at http://dafizilla.sourceforge.net/viewsourcewith/faq-macosx.php for