395090 - remove duplication of pkcs7 code from pkix_pl_httpcertstore.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

pkix_pl_httpcertstore.c has code duped from three of the following files from nss/lib/pkcs7 directory: p7local.c, p7common.c, p7decode.c, certread.c.
The code is used to decode cert(s) data package that comes from CA url found in cert AIA extension.

Currently the pkcs7 code is not linked into nss.so, but linked into separate smime library. So we need to find a solution to have one copy of the pkcs7 code.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

There are many ways to make the code that libPKIX wants available to be 
called without duplicating it.  The duplication MUST be eliminated.  
I don't want to see two copies of that code linked in, one in libSMIME 
and one in libNSS3 (or in libnssUtil).  

One option is to MOVE the code from libSMIME to libNSS3 or libnssUtil, 
just like is being done to move other code from libNSS3 to libUtil.

Another option is to write "stub" functions that load and call the code 
in libSMIME, similar to the way that libSoftoken loads and runs code in 
libFreebl.  

Still another option is to have a function in libNSS that can be 
called by some code outside of libNSS (either in an application, or in 
libSMIME) to register the addresses of the needed libSMIME functions as 
callbacks in NSS, so that libNSS can call them as callbacks.  But I think 
that has problems unless it can be made transparent to applications.

Comment 2

[Julien Pierre]

Nelson,

I agree that we should always avoid duplicating the source.
Duplicating the binary may be acceptable if the object involved is small, as might be the case here. Alexei told me there was only one source file involved.
I am not sure if moving this code to util makes much sense.

Writing stub functions could work, except that there is no initialization function for libsmime. The callback approach works too, but I am afraid the effort to do that will outweigh the benefits vs just compiling one source file twice.

If we really want to move things around, I would rather move all of libsmime into libnss, and have libsmime only be stubs that call into the implementation in libnss. That makes more sense than to move part of libpkcs7 into libnssutil.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Julien, 
I agree that we should not move part of libPKCS7 into libnss or libnssutil.

I do not agree that it is possible that only one little source file from 
libSMIME needs to be duplicated in libNSS.  The function alexei wants to 
call is a function that calls the old PKCS7 decoder.  It depends on the 
entire PKCS7 decoder, which is in libSMIME.  If you bring only that function
over into libNSS, then libNSS is still dependent on the PKCS7 decoder. 

Is a circular dependency being designed here?  I hope not.

Comment 4

[Robert Relyea]

Do we have some test cases for loading pkcs7 from http? I have a patch which I'd like to test before I submit it.

bob

Comment 5

[Robert Relyea]

Attachment: Remove duplicate pkcs7 code and dymically call into libsmime if we need it.

This code replaces the copied pkcs 7 code with a call into the smime to handle try to handle pkcs7 blobs.

the smime code is set up on the fly so that we don't actually load libsmime into the application address space if it's not needed.

pr_callonce is used to handle thread interaction, removing the need for locks.

I added one new error code (which will have to be rolled up in the new error handling stuff) which is triggered if we can't load libsmime3.so.

I grab libsmime3 from the ld_library path (or whatever rules tha platform has for relative path library loading), rather than our extensive code that tries to get 'the right one'. If we want to do the latter, I strongly suggest we put the 'find the right library' code into libutil.

I also added a new function to shutdown to free up the library pointers when we are through.

Comment 6

[Alexei Volkov]

Comment on attachment 282777 [details] [diff] [review]
Remove duplicate pkcs7 code and dymically call into libsmime if we need it.

requesting review from Nelson

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 282777 [details] [diff] [review]
Remove duplicate pkcs7 code and dymically call into libsmime if we need it.

I want Alexei to review this patch for correct usage of the error macros.
He's changing the error handling stuff, and I want him to be sure that
the new error paths in this patch work in a manner consistent with the 
new ways, not the old ones.

I have a bunch of review comments.  Mostly minor stuff.  But they add up to r-.

1. The line shown below must be added to config.mk, not manifest.mn.

>Index: pkix_pl_nss/module/manifest.mn

>+DEFINES += -DSHLIB_SUFFIX=\"$(DLL_SUFFIX)\" -DSHLIB_PREFIX=\"$(DLL_PREFIX)\" -DSHLIB_VERSION=\"$(LIBRARY_VERSION)\"


2.  Nit.  Don't initialize these static values to zero.
Given that they're static, the c language guarantees that they'll be
initialized to zero if you don't explicitly initalize them.  And if you
explicitly initialize them, then that extra unnecessary initialization
data goes into the data segment (or text segment) instead of the bss
segment.  This causes bloat.  Since mozilla is very sensitive to code
size, we should avoid this when we can.

>+static struct pkix_DecodeFuncStr pkix_decodeFunc = { 0 };
>+static const PRCallOnceType pkix_prestine = {0};

Also, the word is "pristine", not "prestine". :)

3. Don't add new uses of PKIX_PK_NSSCALLRV.

>+        PKIX_PL_NSSCALLRV(HTTPCERTSTORECONTEXT, status, PR_CallOnce,
>+                (&pkix_decodeFunc.once, pkix_getDecodeFunction));

>+        PKIX_PL_NSSCALLRV(HTTPCERTSTORECONTEXT, rv , (*pkix_decodeFunc.func),
>+                (certbuf, certlen, f, arg));

We're trying to get rid of this macro completely.  So, as we edit functions
we remove calls to it.  No new uses of that macro should appear in new code.

4. A few stray debug printfs got left in the code.  Here are two I caught.
Please don't leave any of them in.

>@@ -1103,6 +646,7 @@
>         const char *responseData = NULL;
>         PRUint32 responseDataLen = 0;
>         PKIX_List *certList = NULL;
>+printf("********** HttpCertStore called\n");

>@@ -1436,6 +980,7 @@
>         char *path = NULL;
>         PRUint16 port = 0;
>         SECStatus rv = SECFailure;
>+printf("pkix_pl_HttpCertStore_CreateWithAsciiName with location %s\n",locationAscii);

5. I have issues with the name of one new function, and where it is called.
It needs a name that more accurately describes it.

>+void pkix_pl_HttpCertStore_Shutdown(void *plContext);

This function's name suggests that it is a "shutdown" method on an "HttpCertStore"
object.  But it doesn't really shut down any cert store.  It merely unloads the 
PKCS7 library.  I think the name should describe what it does better.

Maybe the new function pkix_pl_HttpCertStore_Shutdown should be called from somewhere
else.  The name of the function pkix_pl_HttpCertStore_Shutdown suggests that it operates
on an HttpCertStore object.  If so, then it probably should be called from inside the 
destructor function for that object.  (Yes, PKIX objects have destructor functions.)

But I'm not sure that the PKIX library inherently limits us to having no more than one
object of the class HttpCertStore.  If not, then we really don't want to unload that
library when the first object of that class gets destroyed.  So, maybe this function
should not be thought of as a method on an HttpCertStore object at all.
Maybe the call is in the right place, and the function simply should be renamed so 
that it no longer appears to be a method on the HttpCertStore class.

Comment 8

[Robert Relyea]

Attachment: V2 address several on nelson's issues.

This patch addresses 3 of the 5 issues. Issues not address:

1) Moving SHLIB defines from manifest.mn.

Why: Primarily to prevent issues with inter-diff, but also: I'd like to know why config.mk is preferrable to manifest.mn. It's adding a new define, which we often do in manifest files. The Variables aren't evaluated until we actually compile, so there isn't an issue with the fact manifest.mn is included before the machine depenencies. It's also where it's added in other directories that use it in NSS. Caveat: I'm don't think it's a big deal to move it to config.mk, so it won't take a very strong argument for me to do it.


2) Shutdown function.
This is really a Shutdown function, not a destructor. The Context that is passed is the global context allocated by libpkix. This function is called once when we shutdown libpkix (more to keep the noise of having a loaded shared library from clouding leak tools than for any other reason.

bob

Comment 9

[Robert Relyea]

Comment on attachment 288739 [details] [diff] [review]
V2 address several on nelson's issues.

Hmm it looks like shutdown moved. Cancelling review until I investigate...

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

> I'd like to know why config.mk is preferrable to manifest.mn. 
> It's adding a new define, which we often do in manifest files.

It's trying to append strings to the DEFINES make variable. 
The DEFINES make variable is defined by coreconf's config.mk, 
which is included AFTER manifest.mn.  Most of the variables defined 
by coreconf's config.mk are defined with = (not += ) so if you 
define a variable in in manifest.mn that is then redefined with =
in coreconf's config.mk, your definition from manifest.mn is lost.
The local directory's config.mk file exists to hold lines that append
strings to make variables defined by coreconf's config.mk.

But in this case, for the make variable DEFINES, I see that coreconf always uses += rather than using =.  So, it's OK to define DEFINES in manifest.mn.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Bob, Don't assume that PKIX always uses a "global context".  
It doesn't always, and I think we're getting rid of it, slowly.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288739 [details] [diff] [review]
V2 address several on nelson's issues.

> it looks like shutdown moved.
Shutdown moved?  What does that mean?

It looks like your latest patch addresses my concerns. So, I'm happy.  But I want to ask Alexei to also take a look.

Comment 13

[Alexei Volkov]

Comment on attachment 288739 [details] [diff] [review]
V2 address several on nelson's issues.

Bob, this patch looks good to me.

Comment 14

[Robert Relyea]

2007-11-26 16:17	rrelyea%redhat.com 	mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h 	1.13 	1/0  	
2007-11-26 16:17	rrelyea%redhat.com 	mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/manifest.mn 	1.7 	3/0 
2007-11-26 16:17	rrelyea%redhat.com 	mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_httpcertstore.c 	1.9 	55/706 
2007-11-26 16:17	rrelyea%redhat.com 	mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_httpcertstore.h 	1.6 	2/49 
2007-11-26 16:17	rrelyea%redhat.com 	mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c 	1.15 	2/0