Bug 1759162 - Fix pkcs11.isModuleInstalled regressions triggered on non-windows builds. r=barret,willdurand
authorLuca Greco <lgreco@mozilla.com>
Tue, 15 Mar 2022 20:13:16 +0000
changeset 610711 9907f16a1b8acd5d4e6e95b99d2f1654ff39258a
parent 610710 6fd4b72c9a6c051be7f5df3774ccbea7394752f9
child 610712 831481b0bc7890b04f5c2ccc8324e6d684791868
push id39424
push usernbeleuzu@mozilla.com
push dateWed, 16 Mar 2022 04:13:25 +0000
treeherdermozilla-central@a2742e46efb5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbarret, willdurand
bugs1759162
milestone100.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1759162 - Fix pkcs11.isModuleInstalled regressions triggered on non-windows builds. r=barret,willdurand PathUtils.filename throws if the path is not an absolute path, which was likely not the case with the OS.Path.basename call used previously. This internal change of behavior shouldn't be triggering any issue on Windows, where the hostInfo.manifest.path seems to be always normalized into an absolute path (by computing it as relative to the hostInfo.manifest if it wasn't already an absolute path), but it makes browser.pkcs11.isModuleInstalled to regress on Linux (and maybe also on MacOS if the pkcs11 manifest files include only the library name and not its full path, as it seems to be the case for the Belgium eID pkcs11 manifest packaged for Linux). The result of PathUtils.filename is expected to only include the basename of the file (without the full dir path and the file extension) and so to fix the regression being triggered on non-windows platform we could use a fake absolute url to get the expected result using PathUtils.filename as is. Differential Revision: https://phabricator.services.mozilla.com/D141097
browser/components/extensions/parent/ext-pkcs11.js
browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js
--- a/browser/components/extensions/parent/ext-pkcs11.js
+++ b/browser/components/extensions/parent/ext-pkcs11.js
@@ -34,29 +34,41 @@ this.pkcs11 = class extends ExtensionAPI
   getAPI(context) {
     let manifestCache = new DefaultMap(async name => {
       let hostInfo = await NativeManifests.lookupManifest(
         "pkcs11",
         name,
         context
       );
       if (hostInfo) {
+        // We don't normalize the absolute path below because
+        // `Path.normalize` throws when the target file doesn't
+        // exist, and that might be the case on non Windows
+        // builds.
+        let absolutePath = PathUtils.isAbsolute(hostInfo.manifest.path)
+          ? hostInfo.manifest.path
+          : PathUtils.joinRelative(
+              PathUtils.parent(hostInfo.path),
+              hostInfo.manifest.path
+            );
+
         if (AppConstants.platform === "win") {
-          // If the path specified in the manifest is not an abslute path,
-          // translate it relative to manifest's directory.
-          if (!PathUtils.isAbsolute(hostInfo.manifest.path)) {
-            hostInfo.manifest.path = PathUtils.normalize(
-              PathUtils.joinRelative(
-                PathUtils.parent(hostInfo.path),
-                hostInfo.manifest.path
-              )
-            );
-          }
+          // On Windows, `hostInfo.manifest.path` is expected to be a normalized
+          // absolute path. On other platforms, this path may be relative but we
+          // cannot use `PathUtils.normalize()` on non-absolute paths.
+          absolutePath = PathUtils.normalize(absolutePath);
+          hostInfo.manifest.path = absolutePath;
         }
-        let manifestLib = PathUtils.filename(hostInfo.manifest.path);
+
+        // PathUtils.filename throws if the path is not an absolute path.
+        // The result is expected to be the basename of the file (without
+        // the dir path and the extension) so it is fine to use an absolute
+        // path that may not be normalized (non-Windows platforms).
+        let manifestLib = PathUtils.filename(absolutePath);
+
         if (AppConstants.platform !== "linux") {
           manifestLib = manifestLib.toLowerCase(manifestLib);
         }
         if (
           manifestLib !== ctypes.libraryName("nssckbi") &&
           manifestLib !== ctypes.libraryName("osclientcerts") &&
           manifestLib !== ctypes.libraryName("ipcclientcerts")
         ) {
--- a/browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js
@@ -106,16 +106,27 @@ async function setupManifests(modules) {
         `Loading of PKCS#11 modules is not supported on ${AppConstants.platform}`
       );
   }
 }
 
 add_task(async function test_pkcs11() {
   async function background() {
     try {
+      const { os } = await browser.runtime.getPlatformInfo();
+      if (os !== "win") {
+        // Expect this call to not throw (explicitly cover regression fixed in Bug 1759162).
+        let isInstalledNonAbsolute = await browser.pkcs11.isModuleInstalled(
+          "testmoduleNonAbsolutePath"
+        );
+        browser.test.assertFalse(
+          isInstalledNonAbsolute,
+          "PKCS#11 module with non absolute path expected to not be installed"
+        );
+      }
       let isInstalled = await browser.pkcs11.isModuleInstalled("testmodule");
       browser.test.assertFalse(
         isInstalled,
         "PKCS#11 module is not installed before we install it"
       );
       await browser.pkcs11.installModule("testmodule", 0);
       isInstalled = await browser.pkcs11.isModuleInstalled("testmodule");
       browser.test.assertTrue(
@@ -243,16 +254,22 @@ add_task(async function test_pkcs11() {
   await setupManifests([
     {
       name: "testmodule",
       description: "PKCS#11 Test Module",
       path: testmodule,
       id: "pkcs11@tests.mozilla.org",
     },
     {
+      name: "testmoduleNonAbsolutePath",
+      description: "PKCS#11 Test Module",
+      path: ctypes.libraryName("pkcs11testmodule"),
+      id: "pkcs11@tests.mozilla.org",
+    },
+    {
       name: "othermodule",
       description: "PKCS#11 Test Module",
       path: testmodule,
       id: "other@tests.mozilla.org",
     },
     {
       name: "internalmodule",
       description: "Builtin Roots Module",