Closed Bug 1509432 Opened 3 years ago Closed 2 years ago

mp_set_long and mp_set_ulong duplicate code

Categories

(NSS :: Libraries, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mt, Assigned: marcus.apb)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files, 2 obsolete files)

The one could easily call the other.
Priority: -- → P3

:jcj

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

Flags: needinfo?(jjones)
Assignee: nobody → 1991manish.kumar

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() */
Flags: needinfo?(jjones)
Assignee: 1991manish.kumar → nobody

Depends on D25942

Assignee: nobody → marcus.apb
Status: NEW → ASSIGNED
Attachment #9055499 - Attachment description: Bug 1509432 - Removed duplicated code between mp_set_int and mp_set_ulong. Created a gtest for this functions. → Bug 1509432 - Removed duplicated code between mp_set_int and mp_set_ulong.
Attachment #9055555 - Attachment description: Bug 1509432 - Added freebl_gtest to nss.gyp, braces to one-line ifs of mp_set_int and more values for mpi_unittest.cc. → Bug 1509432 - Added freebl_gtest to nss.gyp
Attachment #9055555 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.45
Attachment #9055499 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.