[Mac] Do not unload base::NativeLibary-ies if they contain ObjC segments.

base/native_library_mac.mm

Index: base/native_library_mac.mm
diff --git a/base/native_library_mac.mm b/base/native_library_mac.mm
index eec586b863588d9856cc0221da1e73973e10e8de..bbab6827a307cdf58547180c6d2dd43bfbdca56a 100644
--- a/base/native_library_mac.mm
+++ b/base/native_library_mac.mm
@@ -5,9 +5,15 @@
 #include "base/native_library.h"
 
 #include <dlfcn.h>
+#include <mach/task_info.h>
+#include <mach-o/dyld_images.h>
+#include <mach-o/getsect.h>
+
+#include <Foundation/Foundation.h>

[Mark Mentovai, 2013/03/12 19:19:31]

#import me.

[Robert Sesek, 2013/03/12 20:02:10]

Done.

 
 #include "base/file_util.h"
-#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/mac/mac_util.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/string_util.h"
 #include "base/threading/thread_restrictions.h"
@@ -15,6 +21,57 @@
 
 namespace base {
 
+// In 32-bit images, ObjC can be recognized in __OBJC.__image_info, whereas

[Mark Mentovai, 2013/03/12 19:19:31]

It’s usual to separate the segment and section names with commas, I think. (Like
“man otool” does.)

[Robert Sesek, 2013/03/12 20:02:10]

Done.

+// in 64-bit, the data is in __DATA.__objc_imageinfo.
+static const char kObjCImageInfo32[] = "__image_info";

[Mark Mentovai, 2013/03/12 19:19:31]

Why do you need to check for both flavors? If you’re a 32-bit process, only the
32-bit one is relevant. If you’re a 64-bit process, only the 64-bit one is
relevant. You can have one constant here inside an #ifdef and then the code
below becomes simpler.

[Robert Sesek, 2013/03/12 20:02:10]

Done.

+static const char kObjCImageInfo64[] = "__objc_imageinfo";
+
+static bool ImageContainsObjectiveC(const base::FilePath& library_path) {
+  // Get the dyld info from the kernel, which will contain the array of all
+  // loaded modules.
+  task_dyld_info_data_t task_dyld_info;
+  mach_msg_type_number_t count = TASK_DYLD_INFO_COUNT;
+  kern_return_t result = task_info(mach_task_self(), TASK_DYLD_INFO,
+      reinterpret_cast<task_info_t>(&task_dyld_info), &count);
+  if (result != KERN_SUCCESS) {
+    VLOG(1) << "Failed to get TASK_DYLD_INFO: " << result;
+    return false;
+  }
+
+  const char* const library_path_ascii = library_path.MaybeAsASCII().c_str();

[Mark Mentovai, 2013/03/12 19:19:31]

This is bogus since you know what platform you’re on in this file. Just use
library_path.value().c_str(). Less object code that way, too. I’d rename the
variable to “library_path_c” in that case.

[Robert Sesek, 2013/03/12 20:02:10]

Done.

+  VLOG(2) << "Looking up image data for " << library_path_ascii;

[Mark Mentovai, 2013/03/12 19:19:31]

Is this line even needed now?

[Robert Sesek, 2013/03/12 20:02:10]

Removed.

+
+  // Search the array of loaded images for the |library_path| that is being
+  // tested for ObjC.
+  struct dyld_all_image_infos* infos =
+      reinterpret_cast<struct dyld_all_image_infos*>(
+          task_dyld_info.all_image_info_addr);
+  for (uint32_t i = 0; i < infos->infoArrayCount; ++i) {
+    const struct dyld_image_info image_info = infos->infoArray[i];
+    if (strcmp(image_info.imageFilePath, library_path_ascii) != 0)
+      continue;
+
+    // See if the the image contains an "ObjC image info" segment. This method
+    // of testing is copied from _CFBundleGrokObjcImageInfoFromFile in
+    // CF-744/CFBundle.c, around lines 2447-2474.
+    if (image_info.imageLoadAddress->cputype == CPU_TYPE_X86_64) {

[Mark Mentovai, 2013/03/12 19:19:31]

Evaluating this conditional at runtime is stupid (see above).

[Robert Sesek, 2013/03/12 20:02:10]

Done.

+      const section_64* section = getsectbynamefromheader_64(
+          reinterpret_cast<const struct mach_header_64*>(
+              image_info.imageLoadAddress),
+          SEG_DATA, kObjCImageInfo64);

[Mark Mentovai, 2013/03/12 19:19:31]

kObjCImageInfo64/32 didn’t need to be defined external to this function after
all.

[Robert Sesek, 2013/03/12 20:02:10]

Done.

+      return section != NULL;
+    } else {
+      const section* section = getsectbynamefromheader(
+          image_info.imageLoadAddress, SEG_OBJC, kObjCImageInfo32);
+      return section != NULL;
+    }
+  }
+
+  VLOG(1) << "Could not find image info for " << library_path_ascii;
+
+  return false;

[Scott Hess - ex-Googler, 2013/03/12 20:10:38]

AFAICT, in this case we couldn't find the information, so we don't know if it is
safe or not.  Seems like you need to say "Is definitely safe", "Is definitely
unsafe" or "Hellifiknow".  Or change to "ImageDoesNotContainObjectiveC()".

Also, does this ban all code containing Objective-C, or just code containing
class or category definitions?  Admittedly, covering all Objective-C code
handles the issue, while just hitting classes and categories sounds like
enumerating badness (what about protocols?  Or other things I'm missing).

[Robert Sesek, 2013/03/12 20:19:43]

I see your point, but what should happen in the HellIfIKnowCase? Is it any
different than this?
The docs say any ObjC cannot be unloaded. This is also the check that CFBundle
uses to see if an image contains ObjC, so I think this is fine. Better safe than
sorry.

[Scott Hess - ex-Googler, 2013/03/12 20:43:53]

Am I reading it wrong?  In the HellIfIKnow case, this code cannot conclusively
determine if it is safe to unload the library, but since it returns false the
code will optimistically attempt it.  I think an argument could be made that in
that case it should just be leaked.

Well, either leaked, or it should be a CHECK() so that we can figure out WTF is
happening to these users.
OK, that seems reasonable.  I think it would be fine to allow Objective-C calls
only, though obviously that is a fine line if someone leaps in to do crazy
manual method overrides.

+}
+
 // static
 NativeLibrary LoadNativeLibrary(const base::FilePath& library_path,
                                 std::string* error) {
@@ -27,6 +84,7 @@ NativeLibrary LoadNativeLibrary(const base::FilePath& library_path,
     NativeLibrary native_lib = new NativeLibraryStruct();
     native_lib->type = DYNAMIC_LIB;
     native_lib->dylib = dylib;
+    native_lib->image_path = library_path;
     return native_lib;
   }
   base::mac::ScopedCFTypeRef<CFURLRef> url(
@@ -45,11 +103,28 @@ NativeLibrary LoadNativeLibrary(const base::FilePath& library_path,
   native_lib->type = BUNDLE;
   native_lib->bundle = bundle;
   native_lib->bundle_resource_ref = CFBundleOpenBundleResourceMap(bundle);
+
+  base::mac::ScopedCFTypeRef<CFURLRef> executable_url(
+      CFBundleCopyExecutableURL(bundle));
+  NSURL* executable_url_ns = base::mac::CFToNSCast(executable_url);
+  native_lib->image_path =
+      base::FilePath([[executable_url_ns path] UTF8String]);

[Mark Mentovai, 2013/03/12 19:19:31]

Not UTF8String, but fileSystemRepresentation.

[Robert Sesek, 2013/03/12 20:02:10]

Done.

   return native_lib;
 }
 
 // static
 void UnloadNativeLibrary(NativeLibrary library) {
+  if (ImageContainsObjectiveC(library->image_path)) {

[Scott Hess - ex-Googler, 2013/03/12 20:10:38]

Is there any downside to looking this up at Load and adding a "NOUNLOAD" to the
type enum?

[Robert Sesek, 2013/03/12 20:19:43]

We think alike ;). But that's not possible because we don't actually load the
executable manually - we wait for GetFunctionPointerFromNativeLibrary and let
CFBundle do it lazily. Therefore we have to do it at unload, otherwise it's not
loaded and we can't get its info.

[Scott Hess - ex-Googler, 2013/03/12 20:43:53]

Sigh.  Is there any case which differs from:
Line 1: Lazy-load the library.
Line 2: Force the library to be loaded.
??? 

I hate lazy, it just means all of your errors are in the future, after all of
your error-checking is already done.  It should be non-lazy unless you
explicitly ask for it.

+    VLOG(2) << "Not unloading NativeLibrary at "
+            << library->image_path.MaybeAsASCII() << " because it contains "
+            << "an Objective-C segment.";

[Mark Mentovai, 2013/03/12 19:19:31]

Simplify the object code by coalescing these last two static const char[]s into
one. (It’ll all fit on one line.)

[Robert Sesek, 2013/03/12 20:02:10]

Done.

+    // Deliberately do not CFRelease the bundle or dlclose the dylib because
+    // doing so can corrupt the ObjC runtime method caches. See
+    // http://crbug.com/172319 for details.
+    delete library;

[Mark Mentovai, 2013/03/12 19:19:31]

Why don’t you share the “delete” with the delete below, and get rid of the early
return, by flipping the logic?

  if (!ImageContainsObjectiveC(…)) {
    // CFBundleCloseBundleResourceMap/CFRelease or dlclose
  }
  delete library;

[Robert Sesek, 2013/03/12 20:02:10]

Done.

+    return;
+  }
+
   if (library->type == BUNDLE) {
     CFBundleCloseBundleResourceMap(library->bundle,
                                    library->bundle_resource_ref);