1220683 - Checking return values in mpi.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

mpi.c fails to check several return (error) values. This is captured in coverity bugs 221850, 221851, 221853, 1121981, 221854, 221855, 221856, 221857.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: mpi coverity

Comment 2

[Franziskus Kiefer [:franziskus]]

patch is also here https://codereview.appspot.com/270610043

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: mpi coverity

forgot to include assert

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8682444 [details] [diff] [review]
mpi coverity

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

::: lib/freebl/mpi/mpi-priv.h
@@ +15,5 @@
>  #include "mpi.h"
>  #include <stdlib.h>
>  #include <string.h>
>  #include <ctype.h>
> +#include <assert.h>

I'd move the inclusion here to the .c file.

::: lib/freebl/mpi/mpi.c
@@ +1702,5 @@
>  
>    ARGCHK(a != NULL, MP_EQ);
>    
> +  assert(mp_init(&tmp) == MP_OKAY);
> +  assert(mp_set_int(&tmp, z) == MP_OKAY);

It's a shame that you need an alloc() call here.  I wonder if we could hack around the problem somehow.

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: mpi coverity

moved the include. let's leave mp_cmp_int here, I opened bug 1222908 to rewrite that.

Comment 6

[Martin Thomson [:mt:]]

Comment on attachment 8684776 [details] [diff] [review]
mpi coverity

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

::: lib/freebl/mpi/mpi.c
@@ +10,5 @@
>  #include "mpi-priv.h"
>  #if defined(OSF1)
>  #include <c_asm.h>
>  #endif
> +#include <limits.h>

why limits and not assert?

@@ +1491,5 @@
>    mp_set(&s, 1);
>  
>    /* mu = b^2k / m */
> +  MP_CHECKOK(s_mp_add_d(&mu, 1));
> +  MP_CHECKOK(s_mp_lshd(&mu, 2 * USED(m)));

Please try to be consistent with the adjoining code.

Comment 7

[Franziskus Kiefer [:franziskus]]

Attachment: mpi coverity

r+ carries over.

not sure why I used limits here... changed to assert. Also changed MP_CHECKOK to if != MP_OKAY to make it similar to the adjoining code.

do you check in?

Comment 8

[Franziskus Kiefer [:franziskus]]

Attachment: mpi coverity

got distracted... r+ carries over

Comment 9

[Franziskus Kiefer [:franziskus]]

Attachment: mpi coverity

today's not my day... r+ carries over

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8685395 [details] [diff] [review]
mpi coverity

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

https://hg.mozilla.org/projects/nss/rev/1ffeb4a97c1c

Comment 11

[Kai Engert [:KaiE:]]

Martin reported we have test failures:
> cert.sh: #86: Creating EC CA Cert TestCA-ec  (5=0)  - FAILED

Apprently this change broke the NSS tests when using an optimized build (debug works fine).

I will back out the patch.

Comment 12

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #11)
> I will back out the patch.

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

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8685395 [details] [diff] [review]
mpi coverity

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

::: lib/freebl/mpi/mpi.c
@@ +1705,5 @@
>  
>    ARGCHK(a != NULL, MP_EQ);
>    
> +  assert(mp_init(&tmp) == MP_OKAY);
> +  assert(mp_set_int(&tmp, z) == MP_OKAY);

assert() doesn't evaluate the condition at all if NDEBUG is defined, so this would explain the broken opt builds.

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

…except that on Windows, -UNDEBUG is added unconditionally in lib/freebl/mpi/target.mk (and -DNDEBUG is added on Linux, which is arguably worse), so a Windows opt build wouldn't reveal the breakage.  Is there a reason for that?  `hg blame` isn't very helpful.

Comment 15

[Martin Thomson [:mt:]]

Attachment: mp_cmp_int.patch

I'm not sure that this is worth the extra effort involved.  I note that the ARGCHK call returns MP_EQ if the `a` argument is NULL.  Maybe we can dump out with MP_EQ if we have memory issues as well.  It seems like the worst possible return code, but there you have it.

Comment 16

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8688143 [details] [diff] [review]
mp_cmp_int.patch

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

This is a bit overkill, but works, so lets go with this for now (make sure the other patch is checked in again first).

If we could rewrite ec_GFp_pt_dbl_jac [1] we could just remove mp_cmp_int (it's not used anywhere else), but I don't see a quick way to replace the compare there.

[1] https://github.com/nss-dev/nss/blob/79dd4fc2d52149f59824df5013ecf20fa247ba93/lib/freebl/ecl/ecp_jac.c#L235

::: lib/freebl/mpi/mpi.c
@@ +1723,5 @@
>  /*
>    This just converts z to an mp_int, and uses the existing comparison
>    routines.  This is sort of inefficient, but it's not clear to me how
>    frequently this wil get used anyway.  For small positive constants,
>    you can always use mp_cmp_d(), and for zero, there is mp_cmp_z().

you might want to change the comment here

@@ +1733,3 @@
>  
>    ARGCHK(a != NULL, MP_EQ);
>    

nit: whitespace

Comment 17

[Martin Thomson [:mt:]]

Well, ec_GFp_pt_dbl_jac could just do the basic check.

MP_SIGN(a) && MP_DIGIT(a, 0) == 3 && for(i = 1; i < MP_USED(a); ++i) !MP_DIGIT(a, i);

Bob, maybe you can offer an opinion here.  Do you think we need a generic function, or should I just write the above into ec_GFp_pt_dbl_jac ?

Comment 18

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8688143 [details] [diff] [review]
mp_cmp_int.patch

moved this to Bug 1222908 to clean up and land the main patch again without asserts in mp_cmp_int.

Comment 19

[Franziskus Kiefer [:franziskus]]

landed it again without the asserts. let's fix the mp_cmp_int in bug 1222908

https://hg.mozilla.org/projects/nss/rev/973d29c1864f