1256296 - Fix scan-build error in mpmontg.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Richard Barnes [:rbarnes]]

EKR's static analysis on NSS turned up a possible null pointer access in the Cachebleed fix in mpmontg.c.  I don't think this is actually an issue in practice; i.e., things are more coupled than the analyzer thinks they are.  But it makes sense to fix just to keep things clean.

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: 0001-Bug-1256296-Fix-scan-build-error-in-mpmontg.c.patch

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8730185 [details] [diff] [review]
0001-Bug-1256296-Fix-scan-build-error-in-mpmontg.c.patch

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

I agree, this shouldn't happen so an assert is fine.

But maybe this could be rewritten to make the code more readable. Instead of two separate parameters window_bits and num_powers handing over only window_bits seems sufficient. Computing num_powers in mp_exptmod_safe_i would make it easier to understand why this case shouldn't happen imho. (And maybe that would even convince scan-build, but not so sure about that.)

Comment 3

[Robert Relyea]

Comment on attachment 8730185 [details] [diff] [review]
0001-Bug-1256296-Fix-scan-build-error-in-mpmontg.c.patch

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

r+ rrelyea 

NOTE: this function is using assert rather than PR_Assert(). This is consistent with all other assert usages in mpi, so it's fine.

Comment 4

[Franziskus Kiefer [:franziskus]]

thanks for the review Bob.
I landed the patch as https://hg.mozilla.org/projects/nss/rev/e443377fd356