297600 - check executable format before loading plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

Right now, Mozilla products will load any plugin whether or not it can actually
execute. For example - on Intel Macs, ppc plugins will get loaded even though
they cannot be used. Same things for x86-64 Linux - those machines will load
32-bit plugins they can't execute. We should always check to see that we can
actually use a plugin before we load it.

Comment 1

[Mike Shaver (:shaver emeritus)]

With out-of-process plugins, mind, we _could_ use those plugins on those systems.

Comment 2

[Simon Fraser [no longer active]]

Out-of-process plugins? Do we have a plan for how to do that on the various
platforms?

Comment 3

[Benjamin Smedberg]

Define "loaded"... don't we "load" plugins using PR_LoadLibrary, and won't that
function fail if the component isn't a supported binary format?

Comment 4

[Josh Aas]

I guess by load I mean the following:

1) a plugin should not appear in "about:plugins" unless it is useable.
2) we should not attempt to use a plugin unless it is the right architecture

When those two things are done, then we can close this bug. Where are you getting PR_LoadLibrary from? I 
never mentioned that.

Comment 5

[Benjamin Smedberg]

Do plugins currently appear in about:plugins which are not useable?

PR_LoadLibrary is the call which loads plugins: if PR_LoadLibrary fails, plugins
do not appear in the about:plugins list. If there is a bug here, it is that
PR_LoadLibrary loads DLLs which are an incompatible binary type.

On mac, that is
http://lxr.mozilla.org/mozilla/source/modules/plugin/base/src/nsPluginsDirDarwin.cpp#160
On *nix,
http://lxr.mozilla.org/mozilla/source/modules/plugin/base/src/nsPluginsDirUnix.cpp#322
and following.

Comment 6

[Mike Shaver (:shaver emeritus)]

Do we have a test case that shows this to be broken?  My Linux/x86-64 box is
under the weather right now, but I recall it refusing to load the x86-32 plugins
from my copied profile just fine.

Comment 7

[Josh Aas]

2 test cases

1) Apple Intel Macintoshes running Firefox show PPC plugins in "about:plugins" -
I saw it myself
2) jst said his x86-64 behaved badly in a similar way, he can give more specifics

Comment 8

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

I believe macos, like windows, does not use PR_LoadLibrary for finding the
about:plugins info, although that may have changed with OS X...

Comment 9

[Simon Fraser [no longer active]]

(In reply to comment #8)
> I believe macos, like windows, does not use PR_LoadLibrary for finding the
> about:plugins info, although that may have changed with OS X...

Presumably because you don't want to actually load all the plugins (and start
Java etc) just to display about:plugins. So the plugin code has to make a guess
about which plugins are loadable (ideally, it would ask NSPR, but I don't think
NSPR has such an API).

Comment 10

[Josh Aas]

Attachment: relevant Apple patch

this patch has some code which may be helpful for this problem

Comment 11

[timeless]

Comment on attachment 186349 [details] [diff] [review]
relevant Apple patch

don't checkin tabs

Comment 12

[Josh Aas]

timeless: its not my patch and I didn't ask for reviews. such things can be
discussed when actual reviews happen.

Comment 13

[Mark Mentovai]

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

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Bug 299696 isn't really a duplicate of this bug (bug 297600), though
the two are related.  Bug 299696 actually covers a separate (and more
urgent) problem.

But it looks like both bugs need a single solution, so I'm happy to
continue discussion of bug 299696 here (if that's needed).

Comment 15

[Josh Aas]

Attachment: prelim cleanup v1.0

I sat down and figured out our plugin loading situation. I'd like to clean up
our act in stages, starting with this cleanup patch. It removes an unnecessary
executable header test, and cleans up XP_MAC and TARGET_CARBON stuff. Having
this done first will make my upcoming patches more clear. I have most of them
done already but they need a little more testing and cleanup.

Comment 16

[Josh Aas]

Attachment: test for loadability, fix v2.0

I rewrote one of the patches from Apple because it had more than a few serious
issues (weird memory mgmt, "goto" statements, and unnecessarily complex logic).
This is my improved version, which was inspired by their patch (it uses some of
the their code). It prevents FF from loading plugins it can't use on the Mac.

Comment 17

[Josh Aas]

Note that my test for loadability patch (see comment 16 above) may not prevent
us from loading ALL plugins we shouldn't load. However, it greatly improves our
situation and provides a solid foundation on which to build.

Comment 18

[Josh Aas]

Attachment: test for loadability, fix v2.1

This version fixes a bug in my patch that causes gcc3.3 to barf.

I found a problem with the patch (it prevents some plugins from loading on PPC
that should be able to), so this is just posted for reference. Don't review it
unless you're really bored.

Comment 19

[Josh Aas]

Attachment: test for loadability, fix v3.0

This fix works well on x86 and PPC, gcc4.0 and gcc3.3. Note: this fix is
necessary  for x86 Mac builds, and must make it onto the 1.8 branch.

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 191451 [details] [diff] [review]
prelim cleanup v1.0

>Index: nsPluginHostImpl.cpp
>===================================================================

>+#ifdef XP_MACOSX
> #include <TextServices.h>  // for ::UseInputWindow()
> #endif

We should just include <Carbon/Carbon.h> wherever we need Carbon stuff these
days.

>@@ -4476,9 +4468,6 @@
>       const char mach_o_cookie[] = { 0xFE, 0xED, 0xFA, 0xCE };
>       if (memcmp(magic_cookie, mach_o_cookie, sizeof(mach_o_cookie)) == 0)
>         return PR_TRUE;
>-      const char cfm_cookie[] = { 'J', 'o', 'y', '!', 'p', 'e', 'f', 'f' };
>-      if (memcmp(magic_cookie, cfm_cookie, sizeof(cfm_cookie)) == 0)
>-        return PR_FALSE;

Not sure why you're removing this in this patch (need to see more context, e.g.
diff -u12)

Rest looks fine.

Comment 21

[Josh Aas]

I removed that part of the patch because whether or not the test turns out to be
true, we return PR_FALSE. I have no idea why it is there.

Comment 22

[Simon Fraser [no longer active]]

Comment on attachment 191493 [details] [diff] [review]
test for loadability, fix v3.0

>Index: nsPluginsDirDarwin.cpp

> #include <TextUtils.h>
> #include <Aliases.h>
> #include <string.h>
>+#include <stdio.h>
>+#include <unistd.h>
>+#include <mach-o/loader.h>
>+#include <mach-o/fat.h>

Replace the Carbon headers with <Carbon/Carbon.h> ?

>+static OSErr toCFURLRef(nsIFile* file, CFURLRef& outURL)
>+{
>+  nsCOMPtr<nsILocalFileMac> lfm = do_QueryInterface(file);
>+  if (!lfm)
>+    return -1;
>+  CFURLRef foo;
>+  lfm->GetCFURL(&foo);
>+  outURL = foo;

This is very nasty. GetCFURL() might give you back a null CFURLRef,
but you're stuffing this into a reference, so you're violating the C++
"references cannot be null" rule.

Also, why use OSErr for a result type when you're returning NS_OK or -1?
Just use nsresult.

>+// function to test whether or not this is a loadable mach-o plugin bundle
>+static PRBool IsBinaryFileCompatible(CFURLRef aURL)
>+{
>+  if (aURL) {

Can it be null? It's not a pointer. Also, prefer early returns in cases like
this (makes the code easier to read).


> PRBool nsPluginsDir::IsPluginFile(nsIFile* file)
> {
>-    // look at file's creator/type and make sure it is a code fragment, etc.
>+  // do we have a bundle or not?
>+  PRBool rv = PR_FALSE;

"rv" is usually reserved for nsresult return types, so better to us a more
descriptive variable name.


>     FSSpec spec;
>     OSErr err = toFSSpec(file, spec);

Should probably convert this code to Launch Services (LSCopyItemInfoForURL),
which may in fact handle both the bundle and raw file case for you.

>+  // type/creator codes are OK, now check binary compatability

Why not return PR_FALSE here if the tcCheck failed?

Comment 23

[Simon Fraser [no longer active]]

(In reply to comment #21)
> I removed that part of the patch because whether or not the test turns out to be
> true, we return PR_FALSE. I have no idea why it is there.

Oh right. Weird. cvsblame it?

Comment 24

[Simon Fraser [no longer active]]

Comment on attachment 191493 [details] [diff] [review]
test for loadability, fix v3.0

minus based on earlier comments.

Comment 25

[Josh Aas]

Seawood wrote the test that I took out because it returns PR_FALSE either way. I
think he meant to return PR_TRUE, because we do support CFM plugins in mach-o
binaries on PPC. I am going to change my patch to fix his error instead of
deleting his test. Doesn't matter too much since my next patch will remove that
whole function. I didn't notice his mistake before because I wrote the cleanup
patch before I wrote the other plugin loading patch, where I learned that we do
support CFM plugins in mach-o binaries.

Comment 26

[Josh Aas]

Attachment: prelim cleanup v1.1

Comment 27

[Steven Michaud [:smichaud] (Retired)]

As the code now stands (attachment 191500 [details] [diff] [review]), you may not be able to instantiate
universal-binary or intel-binary XPCOM plugins.  See comment 14 and bug
299696.

(I'm not sure if attachment 191493 [details] [diff] [review] makes up for this.)

Comment 28

[Josh Aas]

Yeah, I'm going to figure something out about universal binaries. I want to get
these checks in place first though.

Comment 29

[Josh Aas]

Attachment: test for loadability, fix v3.1

Comment 30

[Simon Fraser [no longer active]]

Comment on attachment 191500 [details] [diff] [review]
prelim cleanup v1.1

>Index: nsPluginHostImpl.cpp
>===================================================================


>   int fd = open_executable(path);
>   if (fd) {
>-    // open the file, look at the header. if the first 8-bytes are "Joy!peff" then we have
>-    // a CFM/PEFF library, which isn't compatible with MACH-O. If it is 0xfeedface, then it
>-    // is MACH-O. Look in /etc/magic for other valid MACH-O header signatures. Should we
>-    // just use the contents of /etc/magic like the "file" command does? man 1 file for more info.
>+    // check the executable header to see if it is something we can use
>+    // currently we can only use 32-bit mach-o (0xFEEDFACE)
>     char magic_cookie[8];
>     ssize_t n = read(fd, magic_cookie, sizeof(magic_cookie));
>     close(fd);
>@@ -4478,7 +4470,7 @@
>         return PR_TRUE;
>       const char cfm_cookie[] = { 'J', 'o', 'y', '!', 'p', 'e', 'f', 'f' };
>       if (memcmp(magic_cookie, cfm_cookie, sizeof(cfm_cookie)) == 0)
>-        return PR_FALSE;
>+        return PR_TRUE;

I read the surrounding code, and the original code is actually correct here.
IsCompatibleExecutable() is asking whether the executable format of the
application and plugin are the same, and that is used to determine whether we
can  do scripting with the plugin (i.e. whether the browser can all into the
plugin's NSGetFactory() method to get XPCOM C++ interfaces). The code assumes
that the application is Mach-O, so it returns false for CFM plugins, though why
those two lines are there I don't know; the function returns false by default.

So the first patch to remove these two lines was correct (and then this fuction
will have to do the intel/ppc test also later).

Comment 31

[Simon Fraser [no longer active]]

Comment on attachment 191557 [details] [diff] [review]
test for loadability, fix v3.1

>Index: nsPluginsDirDarwin.cpp
>===================================================================

>+  nsCOMPtr<nsILocalFileMac> lfm = do_QueryInterface(file);
>+  if (!lfm)
>+    return NS_ERROR_FAILURE;
>+  CFURLRef foo;
>+  nsresult rv = lfm->GetCFURL(&foo);
>+  if (NS_SUCCEEDED(rv))
>+    outURL = foo;
>+  

I think we could use a better variable name than "foo".

>+// function to test whether or not this is a loadable mach-o plugin bundle

Comment is incorrect given that its returns true for CFM plugins too. Also,
"binary file compatible" is a little misleading; maybe call this 
IsLoadablePlugin() ?

>+static PRBool IsBinaryFileCompatible(CFURLRef aURL)
>+{
>+  if (!aURL)
>+    return PR_FALSE;
>+  
>+  char path[PATH_MAX];
>+  if (CFURLGetFileSystemRepresentation(aURL, TRUE, (UInt8*)path, sizeof(path))) {
>+    UInt32 magic;
>+    int f = open(path, O_RDONLY);
>+    if (f != -1) {
>+      if (read(f, &magic, sizeof(magic)) == sizeof(magic)) {
>+        if ((magic == MH_MAGIC) || (magic == FAT_MAGIC) || (magic == FAT_CIGAM)) {
>+          close(f);
>+          return PR_TRUE;
>+        }
>+#ifdef __POWERPC__
>+        // we allow CFM plugins on ppc
>+        UInt32 magic2;
>+        if (read(f, &magic2, sizeof(magic2)) == sizeof(magic2)) {
>+          UInt32 cfm_header1 = 0x4A6F7921; // 'Joy!'
>+          UInt32 cfm_header2 = 0x70656666; // 'peff'
>+          if (cfm_header1 == magic && cfm_header2 == magic2) {
>+            close(f);
>             return PR_TRUE;
>+          }

This code isn't strictly correct. Nothing in CFM says that the executable code
starts at offset 0 in the data fork of the file (although it normally does).
You should really look at athe 'cfrg' resource to get the correct offset.

>+  CFBundleRef bundle = getPluginBundle(path.get());

Seems odd to use the file path here to get the bundle, but to use CFURLRefs
elsewhere in this function. Why not just get the CRUFLRef once at the top, and
use it to get to the bundle, to get the LSItemInfoRecord, and to call
IsBinaryFileCompatible?

>+  if (bundle) { // bundles have different type/creator requirements than files
>+    UInt32 packageType, packageCreator;
>+    CFBundleGetPackageInfo(bundle, &packageType, &packageCreator);
>+    if (packageType == 'BRPL' || packageType == 'IEPL' || packageType == 'NSPL')
>+      tcCheckPassed = PR_TRUE;
>+  } else {
>+    CFURLRef url;
>+    if (NS_SUCCEEDED(toCFURLRef(file, url))) {
>+      LSItemInfoRecord info;
>+      if (LSCopyItemInfoForURL(url,kLSRequestTypeCreator,&info) == noErr) {
>+        if ((info.filetype == 'shlb' && info.creator == 'MOSS') ||
>+            info.filetype == 'NSPL' ||
>+            info.filetype == 'BRPL' ||
>+            info.filetype == 'IEPL') {
>+          tcCheckPassed = PR_TRUE;
>+        }
>+      }
>+    }
>+  }
>+  if (!tcCheckPassed)
>     return PR_FALSE;
>+  
>+  // type/creator codes are OK, now check binary compatability
>+  if (bundle) {
>+    CFURLRef url = CFBundleCopyExecutableURL(bundle);

Rename this var to "executableURL" to avoid confusion.

>+    if (url) {
>+      isPlugin = IsBinaryFileCompatible(url);
>+      CFRelease(url);
>+    }
>+    CFRelease(bundle);
>+  } else {
>+    CFURLRef url;
>+    if (NS_SUCCEEDED(toCFURLRef(file, url)))
>+      isPlugin = IsBinaryFileCompatible(url);
>+  }
>+  
>+  return isPlugin;
> }

Comment 32

[Josh Aas]

Attachment: prelim cleanup v1.2

I just removed the check again. I didn't know we could even try to get XPCOM
interfaces out of plugins, so I incorrectly assumed that we wouldn't be doing
scripting and that the check was just a redundant general compatability check.

Comment 33

[Josh Aas]

Attachment: test for loadability, fix v3.2

I did some suggested cleanup.

I did not do a 'cfrg' resource check to find out where the CFM executable's
start is, as I don't want to spend time on that now. I have seen the first few
bytes of the most popular CFM plugins and they are all 'joy!peff'. As you said,
anything else is unlikely. If you think it is important, we can file a bug on
it.

Also, I did not clean up the CFURL/file-spec usage. Lots of code in that file
needs similar cleanup. I think it is best now to not rewrite functionality that
already exists until the existing functionality can be updated and everyone can
share it. For example, getPluginBundle() should take a CFURLRef argument, but
then I'd either have to make a function that does the same thing but with a
CFURLRef argument, or replace the existing one and update all of its callers. I
don't want to do that now. We can file a bug on cleaning up stylistically
antiquated utility functions in this file if you wish.

Comment 34

[Simon Fraser [no longer active]]

Comment on attachment 191567 [details] [diff] [review]
prelim cleanup v1.2

r=sfraser but please add a comment above IsCompatibleExecutable() noting that
it's used to test whether we can use XPCOM interfaces from the plugin (which
requires C++ ABI compatibility).

Comment 35

[Simon Fraser [no longer active]]

Comment on attachment 191573 [details] [diff] [review]
test for loadability, fix v3.2

>Index: nsPluginsDirDarwin.cpp
>===================================================================

>+static PRBool IsLoadablePlugin(CFURLRef aURL)
>+{
>+  if (!aURL)
>+    return PR_FALSE;
>+  
>+  char path[PATH_MAX];
>+  if (CFURLGetFileSystemRepresentation(aURL, TRUE, (UInt8*)path, sizeof(path))) {
>+    UInt32 magic;
>+    int f = open(path, O_RDONLY);
>+    if (f != -1) {
>+      if (read(f, &magic, sizeof(magic)) == sizeof(magic)) {
>+        if ((magic == MH_MAGIC) || (magic == FAT_MAGIC) || (magic == FAT_CIGAM)) {
>+          close(f);
>+          return PR_TRUE;
>+        }

This doesn't seem right. Don't we want to #ifdef on the build type (PPC vs.
intel) and check for MH_MAGIC on ppc, and MH_CIGAM on intel? Also, I don't
think that it's enough to simply test FAT_MAGIC or FAT_CIGAM; those just tell
you if the file is a multi-architecture file; it doesn't tell you what
architectures the file contains (e.g. it might be 32-bit PPC and 64-bit ppc). I
think we need to read some more of the Mach headers, and find out of there is a
matching executable inside the file.

Comment 36

[Simon Fraser [no longer active]]

Sorry to keep you working on these patches, Josh, but this stuff really has to
work right. Plugins is a complex and fragile area, and I think we really need to
get it right now so that we don't have ongoing issues going forward when users
start encountering ppc/intel/fat plugins in the field.

Also, you should be testing your patches thoroughly, and have CFM, PPC, Intel
and FAT plugins on your machine to make sure that this all works. Ideally, we'd
have test plugins of each type in the tree.

Comment 37

[Josh Aas]

I understood those issues when I wrote the patches - we indeed would not be
accounting for the possibility of ppc64, we'd be loading x86 binaries on PPC and
PPC on x86 for mach-o. That will be addressed in a forthcoming patch. I'd like
to get this in now, so I can work on top of it. It does not break anything we
are not already broken on.

As I said, there are definitely more problems like that to fix, but I'd rather
be doing smaller diffs.

> Also, you should be testing your patches thoroughly, and have CFM, PPC, Intel
> and FAT plugins on your machine to make sure that this all works.

I have all of these plugins, and every patch I have made that I claimed works
has been tested with each one. Do you have doubts about something I haven't
already said wasn't right yet? At the start of this work, I made a large
collection of plugins and collected the first few bytes of each file, all of
their creator/types...

Comment 38

[Brendan Eich [:brendan]]

Maybe the testing angle to work here is automation, via QMO folks. Cc'ing some.

/be

Comment 39

[Doug Turner (:dougt)]

Comment on attachment 191567 [details] [diff] [review]
prelim cleanup v1.2


the comment which talks about why we are looking for FEEDFACE is pretty terse. 
Why exactly are we testing for this and what does it mean.  Fix that up, or
tell me your next patch will address this issue.

josh says that he has tested all types of plugins with this patch.

Comment 40

[Josh Aas]

Comment on attachment 191567 [details] [diff] [review]
prelim cleanup v1.2

I am indeed going to fix up the comment, and the code, in an upcoming patch.
This is just some cleanup prior to that.

Comment 41

[Josh Aas]

Attachment: test for loadability, v3.3

Same as the last patch but is more strict when checking mach-o plugin
architectures.

Comment 42

[Josh Aas]

I am going to port at least the test for loadability patch to the 1.0 branch,
but that patch will not allow Universal Binary plugins to load. That way if we
release another 1.0.x, people won't crash when they install universal binary
plugins. They just won't work.

Note that FF 1.5 should not ship with the current test for loadability patch as
it allows Universal Binary Plugins to load. If we don't actually have a patch to
get Universal Binary plugins working, then 1.5 builds will crash on them.

I will disallow Universal Binary plugin loading before we ship if we do not have
a fix for Universal Binaries done by then.

Comment 43

[Simon Fraser [no longer active]]

Comment on attachment 191652 [details] [diff] [review]
test for loadability, v3.3

>Index: nsPluginsDirDarwin.cpp
>===================================================================

>+PRBool nsPluginsDir::IsPluginFile(nsIFile* file)
>+{
>+  PRBool isPlugin = PR_FALSE;
>+  PRBool tcCheckPassed = PR_FALSE;
>+  
>+  // check type/creator codes
>+  nsCString path;
>+  file->GetNativePath(path);
>+  CFBundleRef bundle = getPluginBundle(path.get());
>+  if (bundle) { // bundles have different type/creator requirements than files
>+    UInt32 packageType, packageCreator;
>+    CFBundleGetPackageInfo(bundle, &packageType, &packageCreator);
>+    if (packageType == 'BRPL' || packageType == 'IEPL' || packageType == 'NSPL')
>+      tcCheckPassed = PR_TRUE;
>+  } else {
>+    CFURLRef url;
>+    if (NS_SUCCEEDED(toCFURLRef(file, url))) {
>+      LSItemInfoRecord info;
>+      if (LSCopyItemInfoForURL(url,kLSRequestTypeCreator,&info) == noErr) {
>+        if ((info.filetype == 'shlb' && info.creator == 'MOSS') ||
>+            info.filetype == 'NSPL' ||
>+            info.filetype == 'BRPL' ||
>+            info.filetype == 'IEPL') {
>+          tcCheckPassed = PR_TRUE;
>+        }
>+      }
>+    }
>+  }
>+  if (!tcCheckPassed)
>     return PR_FALSE;

You can leak 'bundle' here (since it's only released in the code below).

Comment 44

[Josh Aas]

Attachment: test for loadability, v3.4

fixed memory leak

Comment 45

[Josh Aas]

Attachment: test for loadability, v3.5

Comment 46

[Simon Fraser [no longer active]]

Comment on attachment 191672 [details] [diff] [review]
test for loadability, v3.5

>Index: nsPluginsDirDarwin.cpp
>===================================================================

>+    if (NS_SUCCEEDED(toCFURLRef(file, executableURL))) {
>+      isPlugin = IsLoadablePlugin(url);
>+      CFRelease(executableURL);

Alas, one final bug here - replace 'url' with executableURL. If you want,
here's a cleaner version of the whole function:

PRBool nsPluginsDir::IsPluginFile(nsIFile* file)
{
  CFURLRef pluginURL;
  if (NS_FAILED(toCFURLRef(file, pluginURL)))
    return PR_FALSE;

  PRBool isPluginFile = PR_FALSE;

  CFBundleRef pluginBundle = CFBundleCreate(pluginURL);
  if (pluginBundle) {
    UInt32 packageType, packageCreator;
    CFBundleGetPackageInfo(pluginBundle, &packageType, &packageCreator);
    if ( (packageType == 'BRPL' || packageType == 'IEPL' || packageType ==
'NSPL')) {
      CFURLRef executableURL = CFBundleCopyExecutableURL(pluginBundle);
      if (executableURL) {
	isPluginFile = IsLoadablePlugin(executableURL);
	CFRelease(executableURL);
      }
    }

    CFRelease(pluginBundle);
  }
  else {
    LSItemInfoRecord info;
    if (LSCopyItemInfoForURL(pluginURL, kLSRequestTypeCreator, &info) == noErr)
{
      if ((info.filetype == 'shlb' && info.creator == 'MOSS') ||
	  info.filetype == 'NSPL' ||
	  info.filetype == 'BRPL' ||
	  info.filetype == 'IEPL') {
	isPluginFile = IsLoadablePlugin(pluginURL);
      }
    }
  }

  CFRelease(pluginURL);
  return isPluginFile;
}

Comment 47

[Josh Aas]

> CFBundleRef pluginBundle = CFBundleCreate(pluginURL);

That won't compile. This is the function sig:

CFBundleRef CFBundleCreate(CFAllocatorRef allocator, CFURLRef bundleURL);

Comment 48

[Josh Aas]

Attachment: test for loadability, v3.6

Uses Simon's version of IsPluginFile with some slight modifications.

Comment 49

[Steven Michaud [:smichaud] (Retired)]

+#ifdef IS_BIG_ENDIAN
+        if ((magic == MH_MAGIC) || (magic == FAT_MAGIC)) {
+#else
+        if ((magic == MH_CIGAM) || (magic == FAT_CIGAM)) {
+#endif

This won't work as it stands.  A "fat" (or universal binary) file's signature
is the same on big-endian and little-endian systems -- FAT_MAGIC.

Comment 50

[Simon Fraser [no longer active]]

Comment on attachment 191676 [details] [diff] [review]
test for loadability, v3.6

Looks good!

Comment 51

[Josh Aas]

(In reply to comment #49)
> +#ifdef IS_BIG_ENDIAN
> +        if ((magic == MH_MAGIC) || (magic == FAT_MAGIC)) {
> +#else
> +        if ((magic == MH_CIGAM) || (magic == FAT_CIGAM)) {
> +#endif
> 
> This won't work as it stands.  A "fat" (or universal binary) file's signature
> is the same on big-endian and little-endian systems -- FAT_MAGIC.

I think you're backwards. What I wrote will work for universal binaries, since
the byte order of "FAT_CIGAM" (0xbebafeca) as a UInt32 on a little endian system
will match "CAFEBABE" pulled off disk as a UInt32. FAT_MAGIC will match
"CAFEBABE" pulled off disk on a big endian system.

The problem is with MH_CIGAM. In what I wrote, I basically test for the same
UInt32 off disk whether on a big or little endian system. This is wrong, since
on disk thin mach-o will have a different signature (despite the fact that both
are written to disk in big-endian).

Comment 52

[Josh Aas]

Attachment: test for loadability, v3.7

endian fix for thin mach-o

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 191730 [details] [diff] [review]
test for loadability, v3.7

>+#ifdef IS_BIG_ENDIAN
>+        if ((magic == MH_MAGIC) || (magic == FAT_MAGIC)) {
>+#else
>+        if ((magic == MH_MAGIC) || (magic == FAT_CIGAM)) { 
>+#endif

Based on:
http://developer.apple.com/documentation/DeveloperTools/Conceptual/MachORuntime
/FileStructure/chapter_2.1_section_7.html
the FAT_MAGIC/FAT_CIGAM tests look correct.  I couldn't find any documentation
referring to MH_MAGIC, but that test makes sense as well if those are for
single-architecture binaries.

Comment 54

[Simon Fraser [no longer active]]

Comment on attachment 191730 [details] [diff] [review]
test for loadability, v3.7

>Index: nsPluginsDirDarwin.cpp
>===================================================================

>+// function to test whether or not this is a loadable plugin
>+static PRBool IsLoadablePlugin(CFURLRef aURL)
>+{
>+  if (!aURL)
>+    return PR_FALSE;
>+  
>+  char path[PATH_MAX];
>+  if (CFURLGetFileSystemRepresentation(aURL, TRUE, (UInt8*)path, sizeof(path))) {
>+    UInt32 magic;
>+    int f = open(path, O_RDONLY);
>+    if (f != -1) {
>+      if (read(f, &magic, sizeof(magic)) == sizeof(magic)) {
>+#ifdef IS_BIG_ENDIAN
>+        if ((magic == MH_MAGIC) || (magic == FAT_MAGIC)) {
>+#else
>+        if ((magic == MH_MAGIC) || (magic == FAT_CIGAM)) { 
>+#endif

OK, now that we've all spent hours figuring this out, I think we need some
comments in the code here. Something like:

	// Mach-O headers use the byte ordering of the architecture
	// on which they run, so test against the magic number
	// in the byte order we're compiling for.
	// Fat headers are always big-endian, so compare with
	// the appropriate FAT_ signature.
#ifdef IS_BIG_ENDIAN
	if ((magic == MH_MAGIC) || (magic == FAT_MAGIC)) {
#else
	if ((magic == MH_MAGIC) || (magic == FAT_CIGAM)) { 
#endif

Or if we used (NXSwapBigLongToHost(magic) == FAT_MAGIC), then we could
eliminate the #ifdef. r=sfraser with either of those solutions.

Comment 55

[Steven Michaud [:smichaud] (Retired)]

> I think you're backwards. What I wrote will work for universal binaries,
> since the byte order of "FAT_CIGAM" (0xbebafeca) as a UInt32 on a little
> endian system will match "CAFEBABE" pulled off disk as a UInt32. FAT_MAGIC
> will match "CAFEBABE" pulled off disk on a big endian system.

You're right.  I stand corrected.

Comment 56

[Josh Aas]

Attachment: test for loadability, v3.8

add comment, use PR_ntohl instead of |#ifdef|s

Comment 57

[Josh Aas]

Attachment: test for loadability, v3.9

make comment more precise, clean up close() calls some more

Comment 58

[Mark Mentovai]

Comment on attachment 191749 [details] [diff] [review]
test for loadability, v3.9

Looks good.

There's a warning on the call to toCFURLRef in IsPluginFile, because pluginURL
is uninitialized.  I know it doesn't matter, but humor it by initializing
pluginURL to NULL.

These days, I think #ifdef __ppc__ is preferred to #ifdef __POWERPC__, in
IsLoadablePlugin.  More importantly, I'd rather see the stuff inside that chunk
as an else block.

I don't like the separate  hardcoded values for filetypes for bundles and
vanillas, but I won't make an issue about it because vanillas have an extra
type/creator to check for, and consolidating the list would make the code more
difficult to read.

Comment 59

[Josh Aas]

landed "prelim cleanup v1.2" and "test for loadability, v3.9". Thanks for all
the help Simon. Closing this bug, will file bugs for any more specific issues.