Last Comment Bug 118832 - PSM includes the private NSS header file crmfi.h
: PSM includes the private NSS header file crmfi.h
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Security: UI (show other bugs)
: 1.0 Branch
: All All
: P1 normal (vote)
: psm2.4
Assigned To: Stephane Saux
: John Unruh
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-01-08 13:31 PST by Wan-Teh Chang
Modified: 2008-07-10 02:18 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (3.59 KB, patch)
2003-01-22 17:23 PST, Wan-Teh Chang
kaie: review+
bryner: superreview+
asa: approval1.3+
Details | Diff | Splinter Review
Do not use static_cast to cast a pointer to an incomplete type to a void * pointer (779 bytes, patch)
2003-03-05 21:20 PST, Wan-Teh Chang
kaie: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description Wan-Teh Chang 2002-01-08 13:31:56 PST
The PSM file mozilla/security/manager/ssl/src/nsCrypto.cpp
includes the private NSS header file crmfi.h.  We should
find out whether PSM really needs to include crmfi.h.  If
so, we should eliminate that need, which may require fixing
NSS.
Comment 1 Stephane Saux 2002-01-12 10:42:27 PST
I'll try building without that include and we'll see what happens.
Comment 2 John Unruh 2002-02-19 11:44:27 PST
nsbeta1
Comment 3 Kai Engert (:kaie) 2002-07-16 17:55:28 PDT
I tried. When you remove the include, compilcation fails with an error:

/mozilla/security/manager/ssl/src/nsCrypto.cpp: In function `char *
nsCreateReqFromKeyPairs(struct nsKeyPairInfo *, PRInt32, char *, char *, char *,
class nsNSSCertificate *)':
/mozilla/security/manager/ssl/src/nsCrypto.cpp:1347: aggregate `struct
CRMFCertReqMessages messages' has incomplete type and cannot be initialized
Comment 4 Wan-Teh Chang 2002-07-16 22:34:51 PDT
You should be able to use the CRMF_EncodeCertReqMessages function
to avoid the need for 'CRMFCertReqMessages messages'.

You should be able to replace the SEC_ASN1EncodeItem call on
'messages' by two CRMF_EncodeCertReqMessages calls on
'certReqMsgs'.  You would use the first call to get the length
of the encoded output, allocate a SECItem with a buffer that
size, and use the second call to write the encoded output in
the SECItem.

There may be other solutions.
Comment 5 Wan-Teh Chang 2003-01-22 17:23:43 PST
Created attachment 112357 [details] [diff] [review]
Proposed patch
Comment 6 Wan-Teh Chang 2003-01-22 17:27:13 PST
Comment on attachment 112357 [details] [diff] [review]
Proposed patch

Bob, Kai, could you review this patch?

Kai, could you test the code that's affected?
Comment 7 Wan-Teh Chang 2003-01-22 17:30:11 PST
I forgot to mention that this patch is against Mozilla 1.3a.
Comment 8 Kai Engert (:kaie) 2003-01-22 17:35:28 PST
Thanks for the patch. I will review and test it, but note that I'm not a
superreviewer, we will need to ask one of the general superreviewers.
Comment 9 Kai Engert (:kaie) 2003-01-22 18:16:40 PST
Comment on attachment 112357 [details] [diff] [review]
Proposed patch

r=kaie

Patch looks fine to me. I've tested it and it compiles on Linux. However, I'm
not sure which functionality I'd have to access that uses this portion of the
code.

Wan-Teh, do you think compiling on Linux is sufficient, or should I test on
Windows and Mac, too?
Comment 10 Wan-Teh Chang 2003-01-23 08:22:14 PST
Javi, I have a patch that modifies the nsCreateReqFromKeyPairs
function in nsCrypto.cpp
(http://lxr.mozilla.org/security/ident?i=nsCreateReqFromKeyPairs),
which is used by nsCrypto::GenerateCRMFRequest.  Do you know what
tests we can perform to exercise that code path?
Comment 11 Wan-Teh Chang 2003-01-23 08:26:03 PST
My LXR searches showed that nsCrypto::GenerateCRMFRequest
is not being used by the Mozilla client.  Kai, could you
confirm this?  This would mean we can't test the modified
code using the Mozilla client.
Comment 12 Kai Engert (:kaie) 2003-01-23 08:39:38 PST
Oops, indeed. While the interface is defined in nsIDOMCrypto.idl, I can't find
any callers either, neither in JS or C++.
Comment 13 Javier Delgadillo 2003-01-23 10:18:22 PST
To test this, any page that calls crypto.generateCRMFRequest should do.  I
believe John Unruh's web page has such sites set up.  Failing that, you could
just get another cert from the Corporate CA.
Comment 14 Wan-Teh Chang 2003-02-05 22:18:15 PST
Kai, could you test this patch for me?  Thanks.
Comment 15 Kai Engert (:kaie) 2003-02-18 19:28:33 PST
Wan-Teh, did you see my comment 9, where I say I have already tested that your
patch compiles?

Or did you mean that I should test the functionality?
Comment 16 Kai Engert (:kaie) 2003-02-18 19:29:14 PST
John, do you know which of your test pages access crypto.generateCRMFRequest ?
Comment 17 Wan-Teh Chang 2003-02-18 22:03:28 PST
Kai, I'd like you to test the functionality.  Thanks.
Comment 18 John Unruh 2003-02-19 08:54:04 PST
Getting a dual key uses crypto.generateCRMFRequest
https://pki.mcom.com:1025/ca/DirUserEnrollDual1024.html

View the source code at that page.
Comment 19 Javier Delgadillo 2003-02-19 09:46:27 PST
http://testca.netscape.com/ will also test crypto.generateCRMFRequest
Comment 20 Kai Engert (:kaie) 2003-02-19 13:50:47 PST
The patch seems to work for me. With the patch applied, I was successfully able
to obtain certificates from both sites.
Comment 21 Wan-Teh Chang 2003-02-19 15:47:01 PST
Comment on attachment 112357 [details] [diff] [review]
Proposed patch

Kai, thank you for testing this patch.

Brian, could you review this patch?  PSM is including two
private NSS header files.  One of them (genname.h) is just
unnecessary.  The other (crmfi.h) is for the definition of
an opaque structure (CRMFCertReqMessages).  I found that
the need for the definition for that opaque type can be
eliminated by using a public function
(CRMF_EncodeCertReqMessages).
Comment 22 Wan-Teh Chang 2003-02-19 18:19:10 PST
Comment on attachment 112357 [details] [diff] [review]
Proposed patch

Requesting mozilla 1.3 approval.

Asa, please see comment 21 for a description of the patch.
The risk of this patch is low and is confined to certificate
requests.  Kai has tested the patch against our test
certificate authority.
Comment 23 Asa Dotzler [:asa] 2003-02-20 00:18:31 PST
Comment on attachment 112357 [details] [diff] [review]
Proposed patch

a=asa (on behalf of drivers) for checkin to 1.3
Comment 24 Wan-Teh Chang 2003-02-20 06:57:00 PST
Patch checked in.

Checking in Makefile.in;
/cvsroot/mozilla/security/manager/ssl/src/Makefile.in,v  <--  Makefile.in
new revision: 1.54; previous revision: 1.53
done
Checking in nsCrypto.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsCrypto.cpp,v  <--  nsCrypto.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in nsNSSIOLayer.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp,v  <--  nsNSSIOLayer.
cpp
new revision: 1.87; previous revision: 1.86
done
Comment 25 Jim Dunn 2003-03-05 13:13:46 PST
Have you guys tried this on an HP-UX build?  I ran into problems
on hp 10.20 using aCC A.01.21 (I am trying on 11 with a newer aCC
as I speak).  When I build security/managers/ssl/src/nsCrypto.cpp
I get the following:
Error 744: "nsCrypto.cpp", line 884 # The operand of a static_cast must be a
pointer to or lvalue of a complete class; the actual type was CRMFEncryptedKeyStr *.
     static_cast< void* >(encrKey) );
                          ^^^^^^^    
If I go back and include crmfi.h in nsCrypto.cpp and return the -I for the
nss private header directory it builds.

What I think is happening is that aCC needs to know exactly what
CRMFEncryptedKey is.  Right now all it knows is that
it is a "struct CRMFEncryptedKeyStr" but the definition of such is
not included.
Comment 26 Wan-Teh Chang 2003-03-05 13:28:00 PST
Jim,

Could you ask on HP's cxx-dev mailing list what's the right
way to cast a pointer to an incomplete C struct to a void *
pointer in C++?

I think we can replace the static_cast by either a C-style
cast or reinterpret_cast, but I'd like to hear from a C++
expert what's the right way to do this cast in C++.
Comment 27 John Unruh 2003-03-05 14:03:31 PST
Verified per wtc's comment.
Comment 28 Wan-Teh Chang 2003-03-05 21:20:39 PST
Created attachment 116404 [details] [diff] [review]
Do not use static_cast to cast a pointer to an incomplete type to a void * pointer

aCC on HP-UX does not allow using static_cast to cast a
pointer to an incomplete type to a void * pointer.  With
the removal of the crmfi.h inclusion, CRMFEncryptedKey
becomes an incomplete type.

Turns out that any data pointer can be implicitly
converted to a void * pointer by the compiler, so no cast
is needed.
Comment 29 Kai Engert (:kaie) 2003-03-06 06:15:41 PST
Comment on attachment 116404 [details] [diff] [review]
Do not use static_cast to cast a pointer to an incomplete type to a void * pointer

r=kaie
Comment 30 Wan-Teh Chang 2003-03-06 07:06:38 PST
Comment on attachment 116404 [details] [diff] [review]
Do not use static_cast to cast a pointer to an incomplete type to a void * pointer

Patch checked into the trunk (mozilla 1.4alpha).  Jim,
could you verify it on HP-UX?
Comment 31 Jim Dunn 2003-03-06 07:38:42 PST
VERIFIED on hpux... thanks wtc

Note You need to log in before you can comment on or make changes to this bug.