Bug 1362392 - fix s_mpi_div on 32-bit; fix 32-bit test builds

Differential Revision: https://nss-review.dev.mozaws.net/D320

9 files changed, 209 insertions, 45 deletions

diff --git a/automation/taskcluster/docker-fuzz/setup.sh b/automation/taskcluster/docker-fuzz/setup.sh
index a5a36f674..da1a5d71f 100644
--- a/automation/taskcluster/docker-fuzz/setup.sh
+++ b/automation/taskcluster/docker-fuzz/setup.sh
@@ -22,6 +22,10 @@ apt_packages+=('ninja-build')
 apt_packages+=('pkg-config')
 apt_packages+=('zlib1g-dev')
 
+# 32-bit builds
+apt_packages+=('gcc-multilib')
+apt_packages+=('g++-multilib')
+
 # Latest Mercurial.
 apt_packages+=('mercurial')
 apt-key adv --keyserver keyserver.ubuntu.com --recv-keys 41BD8711B1F0EC2B0D85B91CF59CE3A8323293EE
@@ -31,6 +35,14 @@ echo "deb http://ppa.launchpad.net/mercurial-ppa/releases/ubuntu xenial main" >
 apt-get -y update
 apt-get install -y --no-install-recommends ${apt_packages[@]}
 
+# 32-bit builds
+dpkg --add-architecture i386
+apt-get -y update
+apt-get install -y --no-install-recommends libssl-dev:i386
+
+# 32-bit builds
+ln -s /usr/include/x86_64-linux-gnu/zconf.h /usr/include
+
 # Install LLVM/clang-4.0.
 mkdir clang-tmp
 git clone -n --depth 1 https://chromium.googlesource.com/chromium/src/tools/clang clang-tmp/clang
diff --git a/automation/taskcluster/graph/src/extend.js b/automation/taskcluster/graph/src/extend.js
index 8373c5e6b..2c5aff897 100644
--- a/automation/taskcluster/graph/src/extend.js
+++ b/automation/taskcluster/graph/src/extend.js
@@ -63,11 +63,6 @@ queue.filter(task => {
     if (task.collection == "make") {
       return false;
     }
-
-    // Disable mpi tests for now on 32-bit builds (bug 1362392)
-    if (task.platform == "linux32") {
-      return false;
-    }
   }
 
 
@@ -168,6 +163,7 @@ export default async function main() {
   });
 
   await scheduleFuzzing();
+  await scheduleFuzzing32();
 
   await scheduleTools();
 
@@ -415,6 +411,110 @@ async function scheduleFuzzing() {
   return queue.submit();
 }
 
+async function scheduleFuzzing32() {
+  let base = {
+    env: {
+      ASAN_OPTIONS: "allocator_may_return_null=1:detect_stack_use_after_return=1",
+      UBSAN_OPTIONS: "print_stacktrace=1",
+      NSS_DISABLE_ARENA_FREE_LIST: "1",
+      NSS_DISABLE_UNLOAD: "1",
+      CC: "clang",
+      CCC: "clang++"
+    },
+    features: ["allowPtrace"],
+    platform: "linux32",
+    collection: "fuzz",
+    image: FUZZ_IMAGE
+  };
+
+  // Build base definition.
+  let build_base = merge({
+    command: [
+      "/bin/bash",
+      "-c",
+      "bin/checkout.sh && " +
+      "nss/automation/taskcluster/scripts/build_gyp.sh -g -v --fuzz -m32"
+    ],
+    artifacts: {
+      public: {
+        expires: 24 * 7,
+        type: "directory",
+        path: "/home/worker/artifacts"
+      }
+    },
+    kind: "build",
+    symbol: "B"
+  }, base);
+
+  // The task that builds NSPR+NSS.
+  let task_build = queue.scheduleTask(merge(build_base, {
+    name: "Linux 32 (debug, fuzz)"
+  }));
+
+  // The task that builds NSPR+NSS (TLS fuzzing mode).
+  let task_build_tls = queue.scheduleTask(merge(build_base, {
+    name: "Linux 32 (debug, TLS fuzz)",
+    symbol: "B",
+    group: "TLS",
+    command: [
+      "/bin/bash",
+      "-c",
+      "bin/checkout.sh && " +
+      "nss/automation/taskcluster/scripts/build_gyp.sh -g -v --fuzz=tls -m32"
+    ],
+  }));
+
+  // Schedule tests.
+  queue.scheduleTask(merge(base, {
+    parent: task_build_tls,
+    name: "Gtests",
+    command: [
+      "/bin/bash",
+      "-c",
+      "bin/checkout.sh && nss/automation/taskcluster/scripts/run_tests.sh"
+    ],
+    env: {GTESTFILTER: "*Fuzz*"},
+    tests: "ssl_gtests gtests",
+    cycle: "standard",
+    symbol: "Gtest",
+    kind: "test"
+  }));
+
+  // Schedule fuzzing runs.
+  let run_base = merge(base, {parent: task_build, kind: "test"});
+  scheduleFuzzingRun(run_base, "CertDN", "certDN", 4096);
+  scheduleFuzzingRun(run_base, "QuickDER", "quickder", 10000);
+
+  // Schedule MPI fuzzing runs.
+  let mpi_base = merge(run_base, {group: "MPI"});
+  let mpi_names = ["add", "addmod", "div", "expmod", "mod", "mulmod", "sqr",
+                   "sqrmod", "sub", "submod"];
+  for (let name of mpi_names) {
+    scheduleFuzzingRun(mpi_base, `MPI (${name})`, `mpi-${name}`, 4096, name);
+  }
+  scheduleFuzzingRun(mpi_base, `MPI (invmod)`, `mpi-invmod`, 256, "invmod");
+
+  // Schedule TLS fuzzing runs (non-fuzzing mode).
+  let tls_base = merge(run_base, {group: "TLS"});
+  scheduleFuzzingRun(tls_base, "TLS Client", "tls-client", 20000, "client-nfm",
+                     "tls-client-no_fuzzer_mode");
+  scheduleFuzzingRun(tls_base, "TLS Server", "tls-server", 20000, "server-nfm",
+                     "tls-server-no_fuzzer_mode");
+  scheduleFuzzingRun(tls_base, "DTLS Client", "dtls-client", 20000,
+                     "dtls-client-nfm", "dtls-client-no_fuzzer_mode");
+  scheduleFuzzingRun(tls_base, "DTLS Server", "dtls-server", 20000,
+                     "dtls-server-nfm", "dtls-server-no_fuzzer_mode");
+
+  // Schedule TLS fuzzing runs (fuzzing mode).
+  let tls_fm_base = merge(tls_base, {parent: task_build_tls});
+  scheduleFuzzingRun(tls_fm_base, "TLS Client", "tls-client", 20000, "client");
+  scheduleFuzzingRun(tls_fm_base, "TLS Server", "tls-server", 20000, "server");
+  scheduleFuzzingRun(tls_fm_base, "DTLS Client", "dtls-client", 20000, "dtls-client");
+  scheduleFuzzingRun(tls_fm_base, "DTLS Server", "dtls-server", 20000, "dtls-server");
+
+  return queue.submit();
+}
+
 /*****************************************************************************/
 
 async function scheduleTestBuilds(base, args = "") {
diff --git a/automation/taskcluster/graph/src/try_syntax.js b/automation/taskcluster/graph/src/try_syntax.js
index 00de7e987..eddb7c21a 100644
--- a/automation/taskcluster/graph/src/try_syntax.js
+++ b/automation/taskcluster/graph/src/try_syntax.js
@@ -23,7 +23,7 @@ function parseOptions(opts) {
 
   // Parse platforms.
   let allPlatforms = ["linux", "linux64", "linux64-asan", "win64",
-                      "linux64-make", "linux-make", "linux64-fuzz", "aarch64"];
+                      "linux64-make", "linux-make", "linux-fuzz", "linux64-fuzz", "aarch64"];
   let platforms = intersect(opts.platform.split(/\s*,\s*/), allPlatforms);
 
   // If the given value is nonsense or "none" default to all platforms.
@@ -104,6 +104,7 @@ function filter(opts) {
     let found = opts.platforms.some(platform => {
       let aliases = {
         "linux": "linux32",
+        "linux-fuzz": "linux32",
         "linux64-asan": "linux64",
         "linux64-fuzz": "linux64",
         "linux64-make": "linux64",
@@ -119,7 +120,7 @@ function filter(opts) {
         keep &= coll("asan");
       } else if (platform == "linux64-make" || platform == "linux-make") {
         keep &= coll("make");
-      } else if (platform == "linux64-fuzz") {
+      } else if (platform == "linux64-fuzz" || platform == "linux-fuzz") {
         keep &= coll("fuzz");
       } else {
         keep &= coll("opt") || coll("debug");
diff --git a/cmd/mpitests/mpitests.gyp b/cmd/mpitests/mpitests.gyp
index e594e17b3..346d23131 100644
--- a/cmd/mpitests/mpitests.gyp
+++ b/cmd/mpitests/mpitests.gyp
@@ -31,7 +31,18 @@
     'include_dirs': [
       '<(DEPTH)/lib/freebl/mpi',
       '<(DEPTH)/lib/util',
-    ]
+    ],
+    # This uses test builds and has to set defines for MPI.
+    'conditions': [
+      [ 'target_arch=="ia32"', {
+        'defines': [
+          'MP_USE_UINT_DIGIT',
+          'MP_ASSEMBLY_MULTIPLY',
+          'MP_ASSEMBLY_SQUARE',
+          'MP_ASSEMBLY_DIV_2DX1D',
+        ],
+      }],
+    ],
   },
   'variables': {
     'module': 'nss'
diff --git a/coreconf/fuzz.sh b/coreconf/fuzz.sh
index c3cf8abf7..67cb7f594 100644
--- a/coreconf/fuzz.sh
+++ b/coreconf/fuzz.sh
@@ -24,7 +24,10 @@ if [ "$fuzz_oss" = 1 ]; then
   gyp_params+=(-Dno_zdefs=1 -Dfuzz_oss=1)
 else
   enable_sanitizer asan
-  enable_ubsan
+  # Ubsan doesn't build on 32-bit at the moment. Disable it.
+  if [ "$build_64" = 1 ]; then
+    enable_ubsan
+  fi
   enable_sancov
 fi
 
diff --git a/fuzz/fuzz.gyp b/fuzz/fuzz.gyp
index a7339b78c..ed1f53d58 100644
--- a/fuzz/fuzz.gyp
+++ b/fuzz/fuzz.gyp
@@ -88,6 +88,15 @@
               '-lcrypto',
             ],
           }],
+          # For test builds we have to set MPI defines.
+          [ 'target_arch=="ia32"', {
+            'defines': [
+              'MP_USE_UINT_DIGIT',
+              'MP_ASSEMBLY_MULTIPLY',
+              'MP_ASSEMBLY_SQUARE',
+              'MP_ASSEMBLY_DIV_2DX1D',
+            ],
+          }],
         ],
       },
     },
diff --git a/gtests/freebl_gtest/freebl_gtest.gyp b/gtests/freebl_gtest/freebl_gtest.gyp
index 546e69aa9..d74a39b4f 100644
--- a/gtests/freebl_gtest/freebl_gtest.gyp
+++ b/gtests/freebl_gtest/freebl_gtest.gyp
@@ -29,13 +29,6 @@
         '<(DEPTH)/lib/pki/pki.gyp:nsspki',
         '<(DEPTH)/lib/ssl/ssl.gyp:ssl',
       ],
-      'conditions': [
-        [ 'ct_verif==1', {
-          'defines': [
-            'CT_VERIF',
-          ],
-        }],
-      ],
     },
     {
       'target_name': 'prng_gtest',
@@ -62,7 +55,23 @@
   'target_defaults': {
     'include_dirs': [
       '<(DEPTH)/lib/freebl/mpi',
-    ]
+    ],
+    # For test builds we have to set MPI defines.
+    'conditions': [
+      [ 'ct_verif==1', {
+        'defines': [
+          'CT_VERIF',
+        ],
+      }],
+      [ 'target_arch=="ia32"', {
+        'defines': [
+          'MP_USE_UINT_DIGIT',
+          'MP_ASSEMBLY_MULTIPLY',
+          'MP_ASSEMBLY_SQUARE',
+          'MP_ASSEMBLY_DIV_2DX1D',
+        ],
+      }],
+    ],
   },
   'variables': {
     'module': 'nss'
diff --git a/gtests/freebl_gtest/mpi_unittest.cc b/gtests/freebl_gtest/mpi_unittest.cc
index 1f769c014..059183fb6 100644
--- a/gtests/freebl_gtest/mpi_unittest.cc
+++ b/gtests/freebl_gtest/mpi_unittest.cc
@@ -53,13 +53,39 @@ class MPITest : public ::testing::Test {
     mp_clear(&a);
     mp_clear(&b);
   }
+
+  void TestDiv(const std::string a_string, const std::string b_string,
+               const std::string result) {
+    mp_int a, b, c;
+    MP_DIGITS(&a) = 0;
+    MP_DIGITS(&b) = 0;
+    MP_DIGITS(&c) = 0;
+    ASSERT_EQ(MP_OKAY, mp_init(&a));
+    ASSERT_EQ(MP_OKAY, mp_init(&b));
+    ASSERT_EQ(MP_OKAY, mp_init(&c));
+
+    mp_read_radix(&a, a_string.c_str(), 16);
+    mp_read_radix(&b, b_string.c_str(), 16);
+    mp_read_radix(&c, result.c_str(), 16);
+    EXPECT_EQ(MP_OKAY, mp_div(&a, &b, &a, &b));
+    EXPECT_EQ(0, mp_cmp(&a, &c));
+
+    mp_clear(&a);
+    mp_clear(&b);
+    mp_clear(&c);
+  }
 };
 
 TEST_F(MPITest, MpiCmp01Test) { TestCmp("0", "1", -1); }
 TEST_F(MPITest, MpiCmp10Test) { TestCmp("1", "0", 1); }
 TEST_F(MPITest, MpiCmp00Test) { TestCmp("0", "0", 0); }
 TEST_F(MPITest, MpiCmp11Test) { TestCmp("1", "1", 0); }
+TEST_F(MPITest, MpiDiv32ErrorTest) {
+  TestDiv("FFFF00FFFFFFFF000000000000", "FFFF00FFFFFFFFFF", "FFFFFFFFFF");
+}
 
+#ifdef NSS_X64
+// This tests assumes 64-bit mp_digits.
 TEST_F(MPITest, MpiCmpUnalignedTest) {
   mp_int a, b, c;
   MP_DIGITS(&a) = 0;
@@ -90,6 +116,7 @@ TEST_F(MPITest, MpiCmpUnalignedTest) {
   mp_clear(&b);
   mp_clear(&c);
 }
+#endif
 
 // This test is slow. Disable it by default so we can run these tests on CI.
 class DISABLED_MPITest : public ::testing::Test {};
diff --git a/lib/freebl/mpi/mpi.c b/lib/freebl/mpi/mpi.c
index e4b26453f..f56ab9deb 100644
--- a/lib/freebl/mpi/mpi.c
+++ b/lib/freebl/mpi/mpi.c
@@ -2859,6 +2859,9 @@ void
 s_mp_exch(mp_int *a, mp_int *b)
 {
     mp_int tmp;
+    if (!a || !b) {
+        return;
+    }
 
     tmp = *a;
     *a = *b;
@@ -4164,11 +4167,7 @@ mp_err s_mp_div(mp_int *rem,  /* i: dividend, o: remainder */
                 mp_int *quot) /* i: 0;        o: quotient  */
 {
     mp_int part, t;
-#if !defined(MP_NO_MP_WORD) && !defined(MP_NO_DIV_WORD)
-    mp_word q_msd;
-#else
     mp_digit q_msd;
-#endif
     mp_err res;
     mp_digit d;
     mp_digit div_msd;
@@ -4213,7 +4212,7 @@ mp_err s_mp_div(mp_int *rem,  /* i: dividend, o: remainder */
         MP_USED(&part) = MP_USED(div);
 
         /* We have now truncated the part of the remainder to the same length as
-     * the divisor. If part is smaller than div, extend part by one digit. */
+         * the divisor. If part is smaller than div, extend part by one digit. */
         if (s_mp_cmp(&part, div) < 0) {
             --unusedRem;
 #if MP_ARGCHK == 2
@@ -4230,18 +4229,12 @@ mp_err s_mp_div(mp_int *rem,  /* i: dividend, o: remainder */
         div_msd = MP_DIGIT(div, MP_USED(div) - 1);
         if (!partExtended) {
             /* In this case, q_msd /= div_msd is always 1. First, since div_msd is
-       * normalized to have the high bit set, 2*div_msd > MP_DIGIT_MAX. Since
-       * we didn't extend part, q_msd >= div_msd. Therefore we know that
-       * div_msd <= q_msd <= MP_DIGIT_MAX < 2*div_msd. Dividing by div_msd we
-       * get 1 <= q_msd/div_msd < 2. So q_msd /= div_msd must be 1. */
+             * normalized to have the high bit set, 2*div_msd > MP_DIGIT_MAX. Since
+             * we didn't extend part, q_msd >= div_msd. Therefore we know that
+             * div_msd <= q_msd <= MP_DIGIT_MAX < 2*div_msd. Dividing by div_msd we
+             * get 1 <= q_msd/div_msd < 2. So q_msd /= div_msd must be 1. */
             q_msd = 1;
         } else {
-#if !defined(MP_NO_MP_WORD) && !defined(MP_NO_DIV_WORD)
-            q_msd = (q_msd << MP_DIGIT_BIT) | MP_DIGIT(&part, MP_USED(&part) - 2);
-            q_msd /= div_msd;
-            if (q_msd == RADIX)
-                --q_msd;
-#else
             if (q_msd == div_msd) {
                 q_msd = MP_DIGIT_MAX;
             } else {
@@ -4249,7 +4242,6 @@ mp_err s_mp_div(mp_int *rem,  /* i: dividend, o: remainder */
                 MP_CHECKOK(s_mpv_div_2dx1d(q_msd, MP_DIGIT(&part, MP_USED(&part) - 2),
                                            div_msd, &q_msd, &r));
             }
-#endif
         }
 #if MP_ARGCHK == 2
         assert(q_msd > 0); /* This case should never occur any more. */
@@ -4259,15 +4251,15 @@ mp_err s_mp_div(mp_int *rem,  /* i: dividend, o: remainder */
 
         /* See what that multiplies out to                   */
         mp_copy(div, &t);
-        MP_CHECKOK(s_mp_mul_d(&t, (mp_digit)q_msd));
+        MP_CHECKOK(s_mp_mul_d(&t, q_msd));
 
         /*
-       If it's too big, back it off.  We should not have to do this
-       more than once, or, in rare cases, twice.  Knuth describes a
-       method by which this could be reduced to a maximum of once, but
-       I didn't implement that here.
-     * When using s_mpv_div_2dx1d, we may have to do this 3 times.
-     */
+           If it's too big, back it off.  We should not have to do this
+           more than once, or, in rare cases, twice.  Knuth describes a
+           method by which this could be reduced to a maximum of once, but
+           I didn't implement that here.
+           When using s_mpv_div_2dx1d, we may have to do this 3 times.
+         */
         for (i = 4; s_mp_cmp(&t, &part) > 0 && i > 0; --i) {
             --q_msd;
             MP_CHECKOK(s_mp_sub(&t, div)); /* t -= div */
@@ -4282,11 +4274,11 @@ mp_err s_mp_div(mp_int *rem,  /* i: dividend, o: remainder */
         s_mp_clamp(rem);
 
         /*
-      Include the digit in the quotient.  We allocated enough memory
-      for any quotient we could ever possibly get, so we should not
-      have to check for failures here
-     */
-        MP_DIGIT(quot, unusedRem) = (mp_digit)q_msd;
+          Include the digit in the quotient.  We allocated enough memory
+          for any quotient we could ever possibly get, so we should not
+          have to check for failures here
+         */
+        MP_DIGIT(quot, unusedRem) = q_msd;
     }
 
     /* Denormalize remainder                */