1509432 - mp_set_long and mp_set_ulong duplicate code

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Martin Thomson [:mt:]]

The one could easily call the other.

Comment 1

[Manish [:manishkk]]

:jcj

What do I need to do exactly in this bug? not clear to me.

Comment 2

[J.C. Jones [:jcj] (he/they)]

I believe what Martin is referring to is that mp_set_int and mp_set_ulong are very similar.

Looking at the diff of them, they're practically the same code. ulong uses labs() to get the absolute long value of z and uses that henceforth, then checks the sign at the end. So that indicates that you could reduce mp_set_int to just getting labs(z) then calling mp_set_ulong on it, and doing the sign check at the end.

It would be very good to write a gtest for this though, too, to confirm that there isn't a regression. Right now https://searchfox.org/mozilla-central/source/security/nss/gtests/freebl_gtest/mpi_unittest.cc doesn't have tests of these functions.

diff --git a/tmp/int b/tmp/ulong
index bf17274..63949a4 100644
--- a/tmp/int
+++ b/tmp/ulong
@@ -1,8 +1,7 @@
 mp_err
-mp_set_int(mp_int *mp, long z)
+mp_set_ulong(mp_int *mp, unsigned long z)
 {
     int ix;
-    unsigned long v = labs(z);
     mp_err res;

     ARGCHK(mp != NULL, MP_BADARG);
@@ -11,21 +10,17 @@ mp_set_int(mp_int *mp, long z)
     if (z == 0)
         return MP_OKAY; /* shortcut for zero */

-    if (sizeof v <= sizeof(mp_digit)) {
-        DIGIT(mp, 0) = v;
+    if (sizeof z <= sizeof(mp_digit)) {
+        DIGIT(mp, 0) = z;
     } else {
         for (ix = sizeof(long) - 1; ix >= 0; ix--) {
             if ((res = s_mp_mul_d(mp, (UCHAR_MAX + 1))) != MP_OKAY)
                 return res;

-            res = s_mp_add_d(mp, (mp_digit)((v >> (ix * CHAR_BIT)) & UCHAR_MAX));
+            res = s_mp_add_d(mp, (mp_digit)((z >> (ix * CHAR_BIT)) & UCHAR_MAX));
             if (res != MP_OKAY)
                 return res;
         }
     }
-    if (z < 0)
-        SIGN(mp) = NEG;
-
     return MP_OKAY;
-
-} /* end mp_set_int() */
+} /* end mp_set_ulong() */

Comment 3

[Marcus Burghardt]

Attachment: Bug 1509432 - Removed duplicated code between mp_set_int and mp_set_ulong.

Comment 4

[Marcus Burghardt]

Attachment: Bug 1509432 - Added freebl_gtest to nss.gyp

Depends on D25942

Comment 5

[Marcus Burghardt]

Attachment: Bug 1509432 - Removed duplicated code between mp_set_int and mp_set_ulong. Created a gtest for this functions. r=KevinJacobs

Comment 6

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/6bef9de7c3de2312deb595f5d1e56c357848518c

Comment 7

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1509432 - clang-format r=jcj

Comment 8

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/daf54f743d3509e2291cb0fd3c1e516c4b43b1e3