1259058 - Use s_mp_setz() to fill the top digits with zeroes after shifting in s_mp_rshd()

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Tim Taubert [:ttaubert] (inactive)]

After shifting an mp_int to the right by |p| digits, s_mp_rshd() reduces USED() by |p| and zeroes out the previously used digits like so:

> MP_USED(mp) -= p;
> while (p-- > 0)
>   *dst++ = 0;

I suggest we could rather use s_mp_setz() here:

> MP_USED(mp) -= p;
> s_mp_setz(MP_DIGITS(mp) + MP_USED(mp), p);

Comment 1

[guptaaditi1709]

I would like to work on this. How should I get started?

Comment 2

[Franziskus Kiefer [:franziskus]]

If you're new to NSS, the first step would be to set up everything and build NSS (see https://github.com/nss-dev/nss/blob/master/readme.md for how to do that). If you have a patch then, you can either upload it here in the bug or on to phabricator https://phabricator.services.mozilla.com/ (which is the preferred method but a little more invovled to set up) and request review on it.

Comment 3

[guptaaditi1709]

I'm new to NSS, I have set up everything. Can you tell which particular file do I need to look up for fixing this?

Comment 4

[Franziskus Kiefer [:franziskus]]

The function s_mp_rshd is here https://searchfox.org/nss/rev/a1d1d15a356dad547752bdc6bd8c1fe2a0e1d327/lib/freebl/mpi/mpi.c#2957.

Comment 5

[guptaaditi1709]

Created a pull request on github: https://github.com/nss-dev/nss/pull/20
@@ -2978,8 +2978,7 @@
 
     MP_USED(mp) -= p;
     /* Fill the top digits with zeroes */
-    while (p-- > 0)
-        *dst++ = 0;
+    s_mp_setz(MP_DIGITS(mp) + MP_USED(mp), p);
 
 } /* end s_mp_rshd() */

Comment 6

[guptaaditi1709]

Attachment: Used s_mp_setz() to fill the top digits with zeroes after shifting in s_mp_rshd()

Comment 7

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8938324 [details] [diff] [review]
Used s_mp_setz() to fill the top digits with zeroes after shifting in s_mp_rshd()

Review of attachment 8938324 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay (holidays and such). To request review you can set the review flag on the uploaded patch.

The patch looks good, but the patch doesn't apply because it has the wrong path. Can you re-generate the patch so it applies? (either hg export or git format-patch -1 <sha>)
Or even better, we usually use phabricator for reviews in NSS. You can use your bugzilla account at https://phabricator.services.mozilla.com/ and use arcanist (https://secure.phabricator.com/book/phabricator/article/arcanist_quick_start/) to upload your patch.

Comment 8

[guptaaditi1709]

Attachment: mpi_patch.patch

Used s_mp_setz() to fill the top digits with zeroes after shifting in s_mp_rshd()

Comment 9

[guptaaditi1709]

Besides also created a pull reguest on GitHub: https://github.com/nss-dev/nss/pull/20 ,if this helps.

Comment 10

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8940410 [details] [diff] [review]
mpi_patch.patch

Review of attachment 8940410 [details] [diff] [review]:
-----------------------------------------------------------------

> Besides also created a pull reguest on GitHub: https://github.com/nss-dev/nss/pull/20 ,if this helps.

Github is a readonly mirror of the official mercurial repository, so unfortunately not.

To request review you have to set the review flag to ? with a name.

This looks like the same patch, it won't apply :(

Comment 11

[Marcus Burghardt]

Here is a good Ansible role for who want to start playing with NSS:
Tip for good-first-bug: https://galaxy.ansible.com/marcusburghardt/ansible_role_nss

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.