Fix native extension lookup

This CL allows native extension lookup to defer to the platform's
library lookup (e.g. dlopen) when the library cannot be found next to
the importing Dart library. It also allows dart-ext: to accept an
absolute path to the native library.

R=asiva@google.com

Review URL: https://codereview.chromium.org/2285223003 .

CHANGELOG.md

@@ -1,5 +1,12 @@
 ## 1.20.0
 
+### Dart VM
+
+* We have improved the way that the VM locates the native code library for a
+  native extension (e.g. `dart-ext:` import). We have updated this
+  [article on native extensions](https://www.dartlang.org/articles/dart-vm/native-extensions)
+  to reflect the VM's improved behavior.
+
 ### Core library changes
 * `dart:core`: Remove deprecated `Resource` class.
   Use the class in `package:resource` instead.

runtime/bin/extensions.cc

@@ -9,6 +9,7 @@
 #include "bin/dartutils.h"
 #include "bin/file.h"
 #include "bin/platform.h"
+#include "bin/utils.h"
 #include "include/dart_api.h"
 #include "platform/assert.h"
 #include "platform/globals.h"
@@ -16,41 +17,100 @@
 namespace dart {
 namespace bin {
 
-Dart_Handle Extensions::LoadExtension(const char* extension_directory,
-                                      const char* extension_name,
-                                      Dart_Handle parent_library) {
-  // For example on Linux: directory/libfoo-arm.so
-  const char* library_strings[] = {
-    extension_directory,  // directory/
-    Platform::LibraryPrefix(),  // lib
-    extension_name,  // foo
-    "-",
-    Platform::HostArchitecture(),  // arm
-    ".",
-    Platform::LibraryExtension(),  // so
-    NULL,
-  };
-  const char* library_file = Concatenate(library_strings);
-  void* library_handle = LoadExtensionLibrary(library_file);
-  if (library_handle == NULL) {
-    // Fallback on a library file name that does not specify the host
-    // architecture. For example on Linux: directory/libfoo.so
-    const char* fallback_library_strings[] = {
-      extension_directory,  // directory/
-      Platform::LibraryPrefix(),  // lib
-      extension_name,  // foo
+static char PathSeparator() {
+  const char* sep = File::PathSeparator();
+  ASSERT(strlen(sep) == 1);
+  return sep[0];
+}
+
+
+void* Extensions::MakePathAndResolve(const char* dir, const char* name) {
+  // First try to find the library with a suffix specifying the architecture.
+  {
+    const char* path_components[] = {
+      dir,
+      Platform::LibraryPrefix(),
+      name,
+      "-",
+      Platform::HostArchitecture(),  // arm
       ".",
       Platform::LibraryExtension(),  // so
       NULL,
     };
-    const char* fallback_library_file = Concatenate(fallback_library_strings);
-    library_handle = LoadExtensionLibrary(fallback_library_file);
-    if (library_handle == NULL) {
-      return GetError();
+    const char* library_file = Concatenate(path_components);
+    void* library_handle = LoadExtensionLibrary(library_file);
+    if (library_handle != NULL) {
+      return library_handle;
     }
   }
 
-  const char* strings[] = { extension_name, "_Init", NULL };
+  // Fall back on a library name without the suffix.
+  {
+    const char* path_components[] = {
+      dir,
+      Platform::LibraryPrefix(),
+      name,
+      ".",
+      Platform::LibraryExtension(),  // so
+      NULL,
+    };
+    const char* library_file = Concatenate(path_components);
+    return LoadExtensionLibrary(library_file);
+  }
+}
+
+
+// IMPORTANT: In the absolute path case, do not extract the filename and search
+// for that by passing it to LoadExtensionLibrary. That can lead to confusion in
+// which the absolute path is wrong, and a different version of the library is
+// loaded from the standard location.
+void* Extensions::ResolveAbsPathExtension(const char* extension_path) {
+  const char* last_slash = strrchr(extension_path, PathSeparator()) + 1;
+  char* name = strdup(last_slash);
+  char* dir = StringUtils::StrNDup(extension_path, last_slash - extension_path);
+  void* library_handle = MakePathAndResolve(dir, name);
+  free(dir);
+  free(name);
+  return library_handle;
+}
+
+
+void* Extensions::ResolveExtension(const char* extension_directory,
+                                   const char* extension_name) {
+  // If the path following dart-ext is an absolute path, then only look for the
+  // library there.
+  if (File::IsAbsolutePath(extension_name)) {
+    return ResolveAbsPathExtension(extension_name);
+  }
+
+  // If the path following dart-ext is just a file name, first look next to
+  // the importing Dart library.
+  void* library_handle = MakePathAndResolve(extension_directory,
+                                            extension_name);
+  if (library_handle != NULL) {
+    return library_handle;
+  }
+
+  // Then pass the library name down to the platform. E.g. dlopen will do its
+  // own search in standard search locations.
+  return MakePathAndResolve("", extension_name);
+}
+
+
+Dart_Handle Extensions::LoadExtension(const char* extension_directory,
+                                      const char* extension_name,
+                                      Dart_Handle parent_library) {
+  void* library_handle = ResolveExtension(extension_directory, extension_name);
+  if (library_handle == NULL) {
+    return GetError();
+  }
+
+  const char* extension = extension_name;
+  if (File::IsAbsolutePath(extension_name)) {
+    extension = strrchr(extension_name, PathSeparator()) + 1;
+  }
+
+  const char* strings[] = { extension, "_Init", NULL };
   const char* init_function_name = Concatenate(strings);
   void* init_function = ResolveSymbol(library_handle, init_function_name);
   Dart_Handle result = GetError();

runtime/bin/extensions.h

@@ -26,6 +26,11 @@ class Extensions {
  private:
   static Dart_Handle GetError();
 
+  static void* MakePathAndResolve(const char* dir, const char* name);
+  static void* ResolveAbsPathExtension(const char* extension_path);
+  static void* ResolveExtension(const char* extensioion_directory,
+                                const char* extension_name);
+
   // The returned string is scope allocated.
   static const char* Concatenate(const char** strings);
 

runtime/bin/loader.cc

@@ -8,6 +8,7 @@
 #include "bin/builtin.h"
 #include "bin/dartutils.h"
 #include "bin/extensions.h"
+#include "bin/file.h"
 #include "bin/lockers.h"
 #include "bin/utils.h"
 
@@ -261,12 +262,10 @@ static bool LibraryHandleError(Dart_Handle library, Dart_Handle error) {
 }
 
 
-static bool IsWindowsHost() {
-#if defined(TARGET_OS_WINDOWS)
-  return true;
-#else  // defined(TARGET_OS_WINDOWS)
-  return false;
-#endif  // defined(TARGET_OS_WINDOWS)
+static bool PathContainsSeparator(const char* path) {
+  return (strchr(path, '/') != NULL) ||
+         ((strncmp(File::PathSeparator(), "/", 1) != 0) &&
+          (strstr(path, File::PathSeparator()) != NULL));
 }
 
 
@@ -300,7 +299,6 @@ bool Loader::ProcessResultLocked(Loader* loader, Loader::IOResult* result) {
     return false;
   }
 
-
   if (result->tag == _Dart_kImportExtension) {
     ASSERT(library_uri != Dart_Null());
     Dart_Handle library = Dart_LookupLibrary(library_uri);
@@ -320,10 +318,10 @@ bool Loader::ProcessResultLocked(Loader* loader, Loader::IOResult* result) {
       lib_path = lib_uri;
     }
     const char* extension_path = DartUtils::RemoveScheme(extension_uri);
-    if (strchr(extension_path, '/') != NULL ||
-        (IsWindowsHost() && strchr(extension_path, '\\') != NULL)) {
+    if (!File::IsAbsolutePath(extension_path) &&
+        PathContainsSeparator(extension_path)) {
       loader->error_ = DartUtils::NewError(
-          "Relative paths for dart extensions are not supported: '%s'",
+          "Native extension path must be absolute, or simply the file name: %s",
           extension_path);
       return false;
     }

runtime/bin/utils.h

@@ -84,6 +84,9 @@ class StringUtils {
                                    intptr_t len = -1,
                                    intptr_t* result_len = NULL);
 
+  // Not all platforms support strndup.
+  static char* StrNDup(const char* s, intptr_t n);
+
  private:
   DISALLOW_ALLOCATION();
   DISALLOW_IMPLICIT_CONSTRUCTORS(StringUtils);

runtime/bin/utils_android.cc

@@ -71,6 +71,11 @@ char* StringUtils::Utf8ToConsoleString(
 }
 
 
+char* StringUtils::StrNDup(const char* s, intptr_t n) {
+  return strndup(s, n);
+}
+
+
 bool ShellUtils::GetUtf8Argv(int argc, char** argv) {
   return false;
 }

runtime/bin/utils_fuchsia.cc

@@ -68,6 +68,11 @@ char* StringUtils::Utf8ToConsoleString(
 }
 
 
+char* StringUtils::StrNDup(const char* s, intptr_t n) {
+  return strndup(s, n);
+}
+
+
 bool ShellUtils::GetUtf8Argv(int argc, char** argv) {
   return false;
 }

runtime/bin/utils_linux.cc

@@ -69,6 +69,11 @@ char* StringUtils::Utf8ToConsoleString(
 }
 
 
+char* StringUtils::StrNDup(const char* s, intptr_t n) {
+  return strndup(s, n);
+}
+
+
 bool ShellUtils::GetUtf8Argv(int argc, char** argv) {
   return false;
 }

runtime/bin/utils_macos.cc

@@ -77,6 +77,11 @@ char* StringUtils::Utf8ToConsoleString(
 }
 
 
+char* StringUtils::StrNDup(const char* s, intptr_t n) {
+  return strndup(s, n);
+}
+
+
 bool ShellUtils::GetUtf8Argv(int argc, char** argv) {
   return false;
 }

runtime/bin/utils_win.cc

@@ -159,6 +159,23 @@ const wchar_t* StringUtilsWin::Utf8ToWide(
 }
 
 
+char* StringUtils::StrNDup(const char* s, intptr_t n) {
+  intptr_t len = strlen(s);
+  if ((n < 0) || (len < 0)) {
+    return NULL;
+  }
+  if (n < len) {
+    len = n;
+  }
+  char* result = reinterpret_cast<char*>(malloc(len + 1));
+  if (result == NULL) {
+    return NULL;
+  }
+  result[len] = '\0';
+  return reinterpret_cast<char*>(memmove(result, s, len));
+}
+
+
 bool ShellUtils::GetUtf8Argv(int argc, char** argv) {
   wchar_t* command_line = GetCommandLineW();
   int unicode_argc;

tests/standalone/io/test_extension_fail_test.dart

@@ -35,37 +35,55 @@ String getExtensionPath(String buildDirectory) {
   }
 }
 
-void main() {
+bool checkExitCode(int code) {
+  return ((code == 255) || (code == 253));
+}
+
+bool checkStdError(String err) {
+  return err.contains("Unhandled exception:") ||
+         err.contains(
+            "Native extension path must be absolute, or simply the file name");
+}
+
+// name is either "extension" or "relative_extension"
+Future test(String name, bool checkForBall) {
   String scriptDirectory = dirname(Platform.script.toFilePath());
   String buildDirectory = dirname(Platform.executable);
   Directory tempDirectory =
-      Directory.systemTemp.createTempSync('dart_test_extension_fail');
+      Directory.systemTemp.createTempSync('dart_test_${name}_fail');
   String testDirectory = tempDirectory.path;
 
   // Copy test_extension shared library, test_extension.dart and
   // test_extension_fail_tester.dart to the temporary test directory.
   copyFileToDirectory(getExtensionPath(buildDirectory),
                       testDirectory).then((_) {
-    var extensionDartFile = join(scriptDirectory, 'test_extension.dart');
+    var extensionDartFile = join(scriptDirectory, 'test_${name}.dart');
     return copyFileToDirectory(extensionDartFile, testDirectory);
   }).then((_) {
     var testExtensionTesterFile =
-        join(scriptDirectory, 'test_extension_fail_tester.dart');
+        join(scriptDirectory, 'test_${name}_fail_tester.dart');
     return copyFileToDirectory(testExtensionTesterFile, testDirectory);
   }).then((_) {
-    var script = join(testDirectory, 'test_extension_fail_tester.dart');
-    return Process.run(Platform.executable, [script]);
+    var script = join(testDirectory, 'test_${name}_fail_tester.dart');
+    return Process.run(Platform.executable, ['--trace-loading', script]);
   }).then((ProcessResult result) {
     print("ERR: ${result.stderr}\n\n");
     print("OUT: ${result.stdout}\n\n");
-    if (result.exitCode != 255) {
-      throw new StateError("bad exit code");
+    if (!checkExitCode(result.exitCode)) {
+      throw new StateError("bad exit code: ${result.exitCode}");
     }
-    if (!result.stderr.contains("Unhandled exception:")) {
+    if (!checkStdError(result.stderr)) {
       throw new StateError("stderr doesn't contain unhandled exception.");
     }
-    if (!result.stderr.contains("ball")) {
-      throw new StateError("stderr doesn't contain 'ball'.");
+    if (checkForBall) {
+      if (!result.stderr.contains("ball")) {
+        throw new StateError("stderr doesn't contain 'ball'.");
+      }
     }
   }).whenComplete(() => tempDirectory.deleteSync(recursive: true));
 }
+
+main() async {
+  await test("extension", true);
+  await test("relative_extension", false);
+}

tests/standalone/io/test_relative_extension.dart

@@ -0,0 +1,20 @@
+// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library test_extension;
+
+import "dart-ext:extension/test_extension";
+
+class Cat {
+  Cat(this.x);
+
+  num x;
+
+  String toString() => 'cat $x';
+
+  // Implements (a != null) ? a : b using a native C++ function and the API.
+  static int ifNull(a, b) native 'TestExtension_IfNull';
+
+  static int throwMeTheBall(ball) native 'TestExtension_ThrowMeTheBall';
+}

tests/standalone/io/test_relative_extension_fail_tester.dart

@@ -0,0 +1,21 @@
+// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library test_extension_test;
+
+import "dart:async";
+import "dart:isolate";
+import "test_relative_extension.dart";
+
+main() {
+  try {
+    Cat.throwMeTheBall("ball");
+  } on String catch (e) {
+    if (e != "ball") throw new StateError("exception not equal to 'ball'");
+  }
+  // Make sure the exception is thrown out to the event handler from C++ code.
+  // The harness expects the string "ball" to be thrown and the process to
+  // end with an unhandled exception.
+  Timer.run(() => Cat.throwMeTheBall("ball"));
+}