625491 - PKCS #11 logger should print CK_ULONG attributes as an integer

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

Right now the PKCS #11 logger print a CK_ULONG attribute
(such as CKA_VALUE_LEN) as a hex dump, for example,

331569088[1bd1610]:     CKA_VALUE_LEN = 3000000000000000 [8]

It should print the CKA_VALUE_LEN attribute as a decimal integer:

331569088[1bd1610]:     CKA_VALUE_LEN = 48

The proposed patch fixes this.  It also fixes the following problems.

1. Change CKA_SUB_PRIME_BITS to CKA_SUBPRIME_BITS (the new name of
the macro).

2. Fix an incorrect fall-through in the cases CKA_ISSUER/CKA_SUBJECT.
The original code falls through to the next case (case CKA_ID),
as opposed to the intended 'default' case at the end.  This
fall-through code was apparently copied from the CKA_ID case
blindly.

I use a goto statement, which is the simplest solution.  Sigh.
(Also, the compiler doesn't allow me to "goto default".)

Comment 1

[Robert Relyea]

Comment on attachment 503622 [details] [diff] [review]
Proposed patch

r+ rrelyea.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 503622 [details] [diff] [review]
Proposed patch

CK_ULONG is a unsigned long, IINM, which (sadly) is machine dependent in size.
NSPR's %d is not machine side dependent, and neither is NSPR's %ld. 
SO, I'm not sure that using %d to print a CK_ULONG is correct.
Other than that, this patch is AOK.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 503622 [details] [diff] [review]
Proposed patch

Wan-Teh,
Please improve this patch by changing
+	    PR_LOG(modlog, 4, (fmt_s_d, 
+	           atype, valueLen));
to
+	    PR_LOG(modlog, 4, (fmt_s_d, atype, (PRUint32)valueLen));

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch v2 (checked in)

I made the changes that Nelson suggested, except that I
use the %lu format to match the PRUint32 type exactly.
(PR_*printf uses 'l' for 32-bit integer types.)

Patch checked in on the NSS trunk (NSS 3.13) and the
NSS_3_12_BRANCH (NSS 3.12.10).

Checking in debug_module.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/debug_module.c,v  <--  debug_module.c
new revision: 1.17; previous revision: 1.16
done

Checking in debug_module.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/debug_module.c,v  <--  debug_module.c
new revision: 1.15.2.2; previous revision: 1.15.2.1
done

Comment 5

[Wan-Teh Chang]

Attachment: Make pk11load.o depend on debug_module.c (checked in)

pk11load.c includes debug_module.c:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11load.c&rev=1.30&mark=52,54,57#52

The makefile needs this dependency so that pk11load.c is
recompiled when debug_module.c changes.

Comment 6

[Robert Relyea]

Comment on attachment 505138 [details] [diff] [review]
Make pk11load.o depend on debug_module.c (checked in)

r+ rrelyea

Comment 7

[Wan-Teh Chang]

Comment on attachment 505138 [details] [diff] [review]
Make pk11load.o depend on debug_module.c (checked in)

Patch checked in on the NSS trunk (NSS 3.13) and the
NSS_3_12_BRANCH (NSS 3.12.10).

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/pk11wrap/Makefile,v  <--  Makefile
new revision: 1.9; previous revision: 1.8
done

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/pk11wrap/Makefile,v  <--  Makefile
new revision: 1.8.66.1; previous revision: 1.8
done