From 4477ba140f97504002b73081c6293a38f99fc54a Mon Sep 17 00:00:00 2001 From: Franziskus Kiefer Date: Tue, 9 May 2017 13:21:55 -0700 Subject: [PATCH] Bug 1362392 - fix s_mpi_div on 32-bit; fix 32-bit test builds Differential Revision: https://nss-review.dev.mozaws.net/D320 --HG-- extra : rebase_source : baeca1884848bc3a5c04fb8476d817afdba68805 extra : amend_source : ab664774ff4125a3450d946aa721017d14324975 extra : histedit_source : ef25f0cc93bf58165aa18b480aa4b557ba4fa70c%2Cf4bf5aabb9bf1b718df5f8dc8683b466b325a997 --- automation/taskcluster/docker-fuzz/setup.sh | 12 ++ automation/taskcluster/graph/src/extend.js | 110 +++++++++++++++++- .../taskcluster/graph/src/try_syntax.js | 5 +- cmd/mpitests/mpitests.gyp | 13 ++- coreconf/fuzz.sh | 5 +- fuzz/fuzz.gyp | 9 ++ gtests/freebl_gtest/freebl_gtest.gyp | 25 ++-- gtests/freebl_gtest/mpi_unittest.cc | 27 +++++ lib/freebl/mpi/mpi.c | 48 ++++---- 9 files changed, 209 insertions(+), 45 deletions(-) diff --git a/automation/taskcluster/docker-fuzz/setup.sh b/automation/taskcluster/docker-fuzz/setup.sh index a5a36f674e..da1a5d71f5 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 8373c5e6b4..2c5aff8975 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 00de7e9872..eddb7c21a8 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 e594e17b36..346d231318 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 c3cf8abf74..67cb7f5949 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 a7339b78ca..ed1f53d585 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 546e69aa95..d74a39b4f8 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 1f769c0145..059183fb6d 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 e4b26453f2..f56ab9deb1 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 */